own: a const declared from a merge of owning locals borrows them; a value moved into a field of an element read borrows that field (TSLANG 58, 59)

TSLANG_ISSUES.md

@@ -353,6 +353,37 @@ own now, and with it `examples/render_octomap.ts`. Six of ntg's `core/` modules
 Regression tests `tslang/test/tester/tests/00tslang_issue_57.ts` (every model, and as an exe) and
 `own/own_err_parameter_let_stored.ts`.
 
+58 (a `const` declared from a merge of owning locals: `const selected = x2py2gez2pw2 ? (x2gey2 ? tensor0 :
+tensor1) : (z2gew2 ? tensor2 : tensor3)` in `core/directx_math.ts`'s quaternion-from-matrix, refused "is
+merged from several branches; -mm=own cannot prove which one owns it yet") is fixed. Even `const s = c ?
+t0 : t1` was refused: each local keeps its value, and is released at its scope's end, so neither can move
+into the merge. The `const` (or `let`) now borrows each of them, as a `let` given one local while that
+local is still used already did: rc's retain and release for it go, and each local's own releases and
+assignments end the borrow. One of them assigned or moved while the `const` is still used, or the
+`const` stored, returned or assigned, is still refused. null or undefined may be one of the ways in. A
+merge made in a loop's body was also refused, "moved inside a loop but was made outside it" (the merge's
+own declaration was taken as reached again around the loop before the merge is remade), which also hit
+54's `const p = c ? mk() : null` in a loop. With `directx_math.ts` compiling, its facts object exists, so
+`core/simple_camera.ts` compiles too (it had been refused for calling into a module with no facts), and
+the sweep went from 141 failing sources to 126. Regression tests `tslang/test/tester/tests/00tslang_issue_58.ts`
+(every model) and `own/own_err_merged_locals_overwritten.ts`, `own/own_err_merged_locals_moved.ts`.
+
+59 (a value moved into a field of an object read out of an array, then used: `const queuedFrame =
+this.queuedFrames[i]; ... queuedFrame.aabbAttributeBuffer = buffer; const view = buffer ?
+buffer.createStructuredView(0) : null;` in `examples/nri/procedural_geometry_nri.ts`, refused "used here
+after its value was moved") is fixed. 50 and 52 follow a value moved into a field, or pushed into a field
+array, by reading the field right after the move; they took only an object read out of a local or
+parameter nothing assigns. A `const` given an element or a field is no local by then (it is the read
+itself), so `frame.view = view` with `const frame = frames[i]` was left a move. The read right after the
+move now takes an object read out of an element or a field too: nothing runs between, so it is the same
+object, and the checks then follow the borrow through that chain. The element or the field it hangs from
+replaced, popped or reset before a use is still refused. In the
+sweep, "used here after its value was moved" went from 49 sites to 37, and `core/ray_tracing_extended_scene.ts`'s
+own errors are left (its `@keeps` and `Shared<T>`, below). One site of this shape is left,
+`procedural_geometry_nri.ts:425` (`!aabbAttributeView` after the `||` chain), which a reduced copy of it
+does not reproduce. Regression tests `tslang/test/tester/tests/00tslang_issue_59.ts` (every model) and
+`own/own_err_element_field_moved_replaced.ts`.
+
 For native_ts_graphics, from the same sweep (its code, not the compiler):
 - `core/ray_tracing_extended_scene.ts`: the exported classes' constructors keep their array parameters
   (`this.origin = origin`), which an exported constructor has to say: `@keeps("origin", "direction")`
@@ -361,8 +392,9 @@ For native_ts_graphics, from the same sweep (its code, not the compiler):
 - `examples/dynamic_primitive_clipping.ts:223`, `desc.setDepthClip(clip)` with `clip` the loop's `int`,
   fails under every model since #846 (a number is not a `boolean` argument, TS2345). alpha127 still
   takes it; the next release won't. `desc.setDepthClip(clip != 0)` fixes it.
-- Under `-mm=own`, 7 `core/` modules do not compile yet (`directx_math.ts`, `donut_first_person_camera.ts`,
-  `nri_donut_textures.ts`, `octomap_common.ts`, `ray_tracing_extended_scene.ts`, `simple_camera.ts`,
-  `rtxpt_game.ts`), and nor do most examples, which #310 did not port: with every facts object built
-  and 50-53 in, 143 of the 276 sources fail. The largest group left is "borrows a field but is used
-  here after it may be released or overwritten" (about 280 sites).
+- Under `-mm=own`, 4 `core/` modules do not compile yet (`donut_first_person_camera.ts`,
+  `nri_donut_textures.ts`, `ray_tracing_extended_scene.ts`, `rtxpt_game.ts`), and nor do many examples,
+  which #310 did not port: with every facts object built and 50-59 in, 126 of the 276 sources fail. The
+  largest group left is "borrows a field but is used here after it may be released or overwritten" (about
+  200 sites), then "used here after its value was moved" (37: mostly a value a pushed object or an
+  earlier call keeps, then returned or used).

tslang/lib/TypeScript/OwnershipFacts.h

@@ -1115,6 +1115,72 @@ inline mlir_ts::LoadOp slotLoadOf(mlir::Value value)
     return loadOp;
 }
 
+// A merge of owning locals' values (TSLANG 58): `const selected = c ? tensor0 : tensor1`. Every way
+// into it brings a read of an owning local (slotLoadOf), null or undefined, or another such merge;
+// `loads` gets the reads. Not across a loop, as isFreshMerge.
+inline bool slotLoadsOfMerge(mlir::BlockArgument argument, llvm::SmallVectorImpl<mlir_ts::LoadOp> &loads, unsigned depth = 0)
+{
+    auto *block = argument.getOwner();
+    if (block->isEntryBlock() || block->hasNoPredecessors() || depth > 8)
+    {
+        return false;
+    }
+
+    mlir::DominanceInfo dominance(block->getParentOp());
+    for (auto predecessor = block->pred_begin(); predecessor != block->pred_end(); ++predecessor)
+    {
+        auto branchOp = mlir::dyn_cast<mlir::BranchOpInterface>((*predecessor)->getTerminator());
+        if (dominance.dominates(block, *predecessor) || !branchOp)
+        {
+            return false;
+        }
+
+        auto operands = branchOp.getSuccessorOperands(predecessor.getSuccessorIndex());
+        if (argument.getArgNumber() < operands.getProducedOperandCount())
+        {
+            return false;
+        }
+
+        auto incoming = operands[argument.getArgNumber()];
+        if (!incoming)
+        {
+            return false;
+        }
+
+        auto constant = incoming;
+        while (auto castOp = constant.getDefiningOp<mlir_ts::CastOp>())
+        {
+            constant = castOp.getIn();
+        }
+
+        if (constant.getDefiningOp<mlir_ts::NullOp>() || constant.getDefiningOp<mlir_ts::UndefOp>() ||
+            constant.getDefiningOp<mlir_ts::OptionalUndefOp>())
+        {
+            continue;
+        }
+
+        if (auto nested = mlir::dyn_cast<mlir::BlockArgument>(incoming))
+        {
+            if (!nested.hasOneUse() || !slotLoadsOfMerge(nested, loads, depth + 1))
+            {
+                return false;
+            }
+
+            continue;
+        }
+
+        auto load = slotLoadOf(incoming);
+        if (!load)
+        {
+            return false;
+        }
+
+        loads.push_back(load);
+    }
+
+    return true;
+}
+
 // The ownership passes ask MLIRTypeHelper about types only, under the default options.
 inline MLIRTypeHelper typeHelper(mlir::MLIRContext *context)
 {

tslang/lib/TypeScript/OwnershipInferenceClosures.cpp

@@ -1906,7 +1906,10 @@ namespace own_inference
     void OwnershipInferencePass::decideSlot(mlir::Value slot, llvm::ArrayRef<SlotReceiver> receivers, llvm::SetVector<mlir::Operation *> &toErase)
     {
         auto owner = varName(slot.getDefiningOp<mlir_ts::VariableOp>());
-        if (receivers.size() == 1 || takeValuesOfTheirOwn(slot, receivers))
+
+        // a `let` declared from a merge of this and other locals borrows it, so nothing moves it (TSLANG 58)
+        auto merged = llvm::any_of(receivers, [&](const SlotReceiver &receiver) { return mergedSlotRetains.contains(receiver.retain); });
+        if (!merged && (receivers.size() == 1 || takeValuesOfTheirOwn(slot, receivers)))
         {
             // a move on some paths only is placed when it is the local's one receiver (spec 30)
             for (auto &receiver : receivers)

tslang/lib/TypeScript/OwnershipInferenceMoves.cpp

@@ -1091,10 +1091,13 @@ namespace own_inference
                 continue;
             }
 
+            // the value is made again before the taker runs again - a merge (a block's argument,
+            // whose definition is its block's first op) made on entering the taker's block, even
+            // when that first op is the taker itself (`const s = c ? t0 : t1` in a loop, TSLANG 58)
             if (kill && kill->getBlock() == block &&
-                (block != top->getBlock() || kill->isBeforeInBlock(top)))
+                (block != top->getBlock() || kill == top || kill->isBeforeInBlock(top)))
             {
-                continue; // the value is made again before the taker runs again
+                continue;
             }
 
             if (block == top->getBlock())

tslang/lib/TypeScript/OwnershipInferencePass.cpp

@@ -393,6 +393,7 @@ namespace own_inference
         fieldBorrowRetains.clear();
         fieldBorrowReleases.clear();
         mergeBorrowRetains.clear();
+        mergedSlotRetains.clear();
         captureStores.clear();
         captureRetains.clear();
         callCopies.clear();
@@ -702,6 +703,27 @@ namespace own_inference
                 continue;
             }
 
+            // a `let` declared from a merge of owning locals (TSLANG 58): it borrows each of them
+            if (auto argument = value ? mlir::dyn_cast<mlir::BlockArgument>(rootOf(value)) : mlir::BlockArgument();
+                argument && mlir::isa<mlir_ts::RetainSlotOp>(op) && borrowerOf(op))
+            {
+                llvm::SmallVector<mlir_ts::LoadOp> loads;
+                if (slotLoadsOfMerge(argument, loads) && !loads.empty())
+                {
+                    mergedSlotRetains.insert(op);
+                    llvm::SmallPtrSet<mlir::Value, 4> slots;
+                    for (auto load : loads)
+                    {
+                        if (slots.insert(load.getReference()).second)
+                        {
+                            slotReceivers[load.getReference()].push_back({op, value, load});
+                        }
+                    }
+
+                    continue;
+                }
+            }
+
             if (value && isReadOutOfException(value))
             {
                 toErase.insert(op);

tslang/lib/TypeScript/OwnershipInferencePass.h

@@ -1104,6 +1104,11 @@ class OwnershipInferencePass
     // decided there
     llvm::DenseSet<mlir::Operation *> mergeBorrowRetains;
 
+    // the `ts.RetainSlot`s of the `let`s declared from a merge of owning locals (TSLANG 58, `const
+    // selected = c ? tensor0 : tensor1`): a receiver of each of those locals, which the `let` borrows
+    // (decideSlot)
+    llvm::DenseSet<mlir::Operation *> mergedSlotRetains;
+
     // A call that keeps an argument and is given the same value again through another operand - the
     // value itself, a view of it, something read out of it, another read of its slot (`f(n, n)`,
     // `f(h, n, n.v)` with only the first `n` kept): the callee takes the value while it borrows it

tslang/lib/TypeScript/OwnershipSignatureFieldLocals.cpp

@@ -119,6 +119,21 @@ namespace own_signature
             return isUnassignedSlot(loadOp.getReference());
         }
 
+        // The object can be read again right after a store into its field or a push into it - nothing
+        // runs between, so a read of an element or of a field is the same object (TSLANG 59: `const
+        // frame = this.frames[i]; frame.view = view;`, with `frame` the element read itself).
+        bool objectReadableRightAfter(mlir_ts::PropertyRefOp field)
+        {
+            if (objectReadable(field))
+            {
+                return true;
+            }
+
+            auto loadOp = field.getObjectRef().getDefiningOp<mlir_ts::LoadOp>();
+            return loadOp && (loadOp.getReference().getDefiningOp<mlir_ts::ElementRefOp>() ||
+                              loadOp.getReference().getDefiningOp<mlir_ts::PropertyRefOp>());
+        }
+
         // `value` seen through the views that keep its block: an optional made of it or opened, a
         // widening or narrowing cast that is a view.
         mlir::Value stripViews(mlir::Value value)
@@ -606,7 +621,7 @@ namespace own_signature
             llvm::SmallVector<mlir_ts::StoreOp> movedStores;
             body->walk([&](mlir_ts::StoreOp storeOp) {
                 auto field = storeOp.getReference().getDefiningOp<mlir_ts::PropertyRefOp>();
-                if (field && objectReadable(field))
+                if (field && objectReadableRightAfter(field))
                 {
                     movedStores.push_back(storeOp);
                 }
@@ -670,7 +685,7 @@ namespace own_signature
             for (auto pushOp : pushes)
             {
                 auto field = pushOp.getOp().getDefiningOp<mlir_ts::PropertyRefOp>();
-                if (pushOp.getItems().size() != 1 || !field || !objectReadable(field))
+                if (pushOp.getItems().size() != 1 || !field || !objectReadableRightAfter(field))
                 {
                     continue;
                 }

tslang/test/tester/CMakeLists.txt

@@ -1911,9 +1911,12 @@ endforeach()
 # TSLANG_ISSUES 55: an array literal of fields, only read, borrows them
 # TSLANG_ISSUES 56: a `let` given only fields borrows each until the next assignment
 # TSLANG_ISSUES 57: a `let` that only holds parameters owns nothing
+# TSLANG_ISSUES 58: a `const` declared from a merge of owning locals borrows them; a merge made in a loop's body
+# TSLANG_ISSUES 59: a value moved into a field of an element or field read, then used, borrows that field
 foreach(issues_mm gc rc none own)
     foreach(issues_test 00tslang_issue_44 00tslang_issue_45 00tslang_issue_46 00tslang_issue_47 00tslang_issue_48 00tslang_issue_49 00tslang_issue_50
-                        00tslang_issue_52 00tslang_issue_53 00tslang_issue_54 00tslang_issue_55 00tslang_issue_56 00tslang_issue_57)
+                        00tslang_issue_52 00tslang_issue_53 00tslang_issue_54 00tslang_issue_55 00tslang_issue_56 00tslang_issue_57
+                        00tslang_issue_58 00tslang_issue_59)
         tslang_add_test(NAME test-jit-${issues_test}-${issues_mm} COMMAND test-runner -jit -mm=${issues_mm} "${PROJECT_SOURCE_DIR}/test/tester/tests/${issues_test}.ts")
         tslang_add_test(NAME test-compile-${issues_test}-${issues_mm} COMMAND test-runner -mm=${issues_mm} "${PROJECT_SOURCE_DIR}/test/tester/tests/${issues_test}.ts")
     endforeach()
@@ -5463,7 +5466,10 @@ set(own_error_cases
     "own_err_merge_moved_twice|:19:9: error: '[^']*' borrows a field and cannot be stored, returned or captured"
     "own_err_borrowing_array_field_reset|:13:16: error: '[^']*' borrows a field but is used here after it may be released or overwritten"
     "own_err_let_borrows_reassigned_overwritten|:17:16: error: '[^']*' borrows a field but is used here after it may be released or overwritten"
-    "own_err_parameter_let_stored|:10:5: error: '[^']*' takes a second reference")
+    "own_err_parameter_let_stored|:10:5: error: '[^']*' takes a second reference"
+    "own_err_merged_locals_overwritten|:11:16: error: '[^']*' borrows '[^']*' but is used here after '[^']*' is released or overwritten"
+    "own_err_merged_locals_moved|:12:5: error: '[^']*' is moved here while '[^']*' borrows it"
+    "own_err_element_field_moved_replaced|:11:12: error: '[^']*' borrows an element but is used here after it may be released or overwritten")
 foreach(own_error_case ${own_error_cases})
     string(FIND "${own_error_case}" "|" own_error_sep)
     string(SUBSTRING "${own_error_case}" 0 ${own_error_sep} own_error_name)

tslang/test/tester/own/own_err_element_field_moved_replaced.ts

@@ -0,0 +1,14 @@
+// TSLANG_ISSUES 59: a value moved into a field of an array's element borrows that field, so the
+// element replaced before a use of the value frees it.
+class View { n = 1; }
+class Frame { view: View | null = null; }
+
+export function f(frames: Frame[]): int {
+    const frame = frames[0];
+    const view = new View();
+    frame.view = view;
+    frames[0] = new Frame();
+    return view.n;
+}
+
+function main() { print(f([new Frame()])); }

tslang/test/tester/own/own_err_merged_locals_moved.ts

@@ -0,0 +1,16 @@
+// TSLANG_ISSUES 58: a `const` declared from a merge of owning locals borrows each of them, so none of
+// them can be moved out while it does.
+function mk(a: number): number[] { return [a, a]; }
+function sum(v: number[]): number { return v[0] + v[1]; }
+
+let kept: number[] = [];
+
+export function f(a: boolean): number {
+    const t0 = mk(1.0);
+    const t1 = mk(2.0);
+    const s = a ? t0 : t1;
+    kept = t0;
+    return sum(s);
+}
+
+function main() { print(f(true)); }

tslang/test/tester/own/own_err_merged_locals_overwritten.ts

@@ -0,0 +1,14 @@
+// TSLANG_ISSUES 58: a `const` declared from a merge of owning locals borrows each of them, so one of
+// them given a new value while it is still used frees what the `const` may hold.
+function mk(a: number): number[] { return [a, a]; }
+function sum(v: number[]): number { return v[0] + v[1]; }
+
+export function f(a: boolean): number {
+    let t0 = mk(1.0);
+    const t1 = mk(2.0);
+    const s = a ? t0 : t1;
+    t0 = mk(3.0);
+    return sum(s);
+}
+
+function main() { print(f(true)); }

tslang/test/tester/tests/00tslang_issue_58.ts

@@ -0,0 +1,78 @@
+// TSLANG_ISSUES 58 (-mm=own), from native_ts_graphics' core/directx_math.ts (XMQuaternionRotationMatrix):
+// `const selected = c1 ? (c2 ? tensor0 : tensor1) : (c3 ? tensor2 : tensor3)` over four owned locals,
+// then only read. Each local keeps its value, so `selected` borrows whichever one it was given - in a
+// loop's body too, and with null as one way in.
+
+function mk(a: number): number[] {
+    return [a, a + 1.0, a + 2.0, a + 3.0];
+}
+
+function sum(v: number[]): number {
+    let s = 0.0;
+    for (let i = 0; i < v.length; i++) {
+        s += v[i];
+    }
+
+    return s;
+}
+
+function scaled(v: number[], s: number): number[] {
+    return [v[0] / s, v[1] / s, v[2] / s, v[3] / s];
+}
+
+export function pick(a: boolean, b: boolean, c: boolean): number[] {
+    const tensor0 = mk(1.0);
+    const tensor1 = mk(2.0);
+    const tensor2 = mk(3.0);
+    const tensor3 = mk(4.0);
+    const selected = a ? (b ? tensor0 : tensor1) : (c ? tensor2 : tensor3);
+    return scaled(selected, sum(selected));
+}
+
+export function total(a: boolean): number {
+    let r = 0.0;
+    for (let i = 0; i < 3; i++) {
+        const t0 = mk(i);
+        const t1 = mk(10.0);
+        const s = a ? t0 : t1;
+        r += sum(s);
+    }
+
+    return r;
+}
+
+export function outer(a: boolean): number {
+    const t0 = mk(1.0);
+    const t1 = mk(2.0);
+    let r = 0.0;
+    for (let i = 0; i < 2; i++) {
+        const s = i == 0 ? t0 : t1;
+        r += sum(s);
+    }
+
+    const n = a ? t0 : null;
+    return n ? r + sum(n) : r;
+}
+
+export function freshInLoop(a: boolean): number {
+    let r = 0.0;
+    for (let i = 0; i < 3; i++) {
+        const p = a ? mk(i) : null;
+        if (p) {
+            r += sum(p);
+        }
+    }
+
+    return r;
+}
+
+function main() {
+    const p = pick(false, true, false);
+    print(p[0], p[3]);
+    assert(p[0] == 4.0 / 22.0 && p[3] == 7.0 / 22.0);
+    assert(pick(true, true, true)[0] == 0.1 && pick(true, false, true)[0] == 2.0 / 14.0);
+    assert(total(true) == 6.0 + 10.0 + 14.0 && total(false) == 3.0 * 46.0);
+    assert(outer(true) == 10.0 + 14.0 + 10.0 && outer(false) == 24.0);
+    assert(freshInLoop(true) == 30.0 && freshInLoop(false) == 0.0);
+    print("done.");
+}