Skip to content

Commit ed0d4f4

Browse files
jtuylsclaude
andauthored
[VM] Fix ref leak from incorrect MOVE bit on branch block args (#23689)
`isLastRealValueUse` checked uses in ALL other blocks when deciding MOVE vs retain for branch operands. Uses in predecessor/definition blocks (prior control-flow edges) incorrectly prevented MOVE, causing ref leaks: retain incremented the ref count but no discard existed on the taken path to release it. Fix: only check uses in immediate successor blocks. Add a conservative guard for values that flow through a successor (liveIn && liveOut) to catch transitive escapes. Signed-off-by: Jorn <jorn.tuyls@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent bef4552 commit ed0d4f4

2 files changed

Lines changed: 88 additions & 9 deletions

File tree

compiler/src/iree/compiler/Dialect/VM/Analysis/ValueLiveness.cpp

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -397,17 +397,42 @@ bool ValueLiveness::isLastRealValueUse(Value value, Operation *useOp,
397397
}
398398
}
399399
// Check if the value escapes to any successor blocks.
400+
// Only check uses in immediate successor blocks. Uses in other blocks
401+
// (e.g. predecessors or the definition block) are on prior control-flow
402+
// edges and don't affect whether MOVE is safe at this branch point.
403+
SmallPtrSet<Block *, 4> successorBlocks;
404+
for (unsigned i = 0; i < useOp->getNumSuccessors(); ++i) {
405+
Block *succBlock = useOp->getSuccessor(i);
406+
successorBlocks.insert(succBlock);
407+
// If the value flows THROUGH a successor (both liveIn and liveOut),
408+
// it reaches non-immediate successors we can't enumerate here.
409+
// Conservatively prevent MOVE. This is precise for current VM pipeline
410+
// invariants: MaterializeRefDiscards places exactly one discard per
411+
// path at value death points, so liveIn && liveOut implies real
412+
// (non-discard) uses downstream.
413+
BlockSets &succSets = blockLiveness_[succBlock];
414+
if (succSets.liveIn.contains(value) && succSets.liveOut.contains(value)) {
415+
return false;
416+
}
417+
}
400418
for (auto &use : value.getUses()) {
401419
Operation *userOp = use.getOwner();
402-
if (userOp->getBlock() != useOp->getBlock()) {
403-
// Use is in another block. If it's a discard AND the value was
404-
// forwarded as a successor operand, skip it (ownership transferred).
405-
// Otherwise, it's a real use and the value escapes.
406-
bool isDiscardOfForwardedValue =
407-
isa<IREE::VM::DiscardRefsOp>(userOp) && valueIsSuccessorOperand;
408-
if (!isDiscardOfForwardedValue) {
409-
return false;
410-
}
420+
Block *userBlock = userOp->getBlock();
421+
if (userBlock == useOp->getBlock()) {
422+
continue;
423+
}
424+
// Skip uses in non-successor blocks (e.g. predecessor or definition
425+
// blocks). These are on prior control-flow edges, not forward ones.
426+
if (!successorBlocks.contains(userBlock)) {
427+
continue;
428+
}
429+
// Use is in a successor block. If it's a discard AND the value was
430+
// forwarded as a successor operand, skip it (ownership transferred).
431+
// Otherwise, it's a real use and the value escapes.
432+
bool isDiscardOfForwardedValue =
433+
isa<IREE::VM::DiscardRefsOp>(userOp) && valueIsSuccessorOperand;
434+
if (!isDiscardOfForwardedValue) {
435+
return false;
411436
}
412437
}
413438
// All uses in other blocks were discards of forwarded values. Fall through

compiler/src/iree/compiler/Dialect/VM/Analysis/test/register_allocation_refs.mlir

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -923,3 +923,57 @@ vm.module @module_scratch_register_swap {
923923
vm.return
924924
}
925925
}
926+
927+
// -----
928+
929+
//===----------------------------------------------------------------------===//
930+
// Regression Test: Ref Forwarded via Block Arg with Predecessor Use
931+
//===----------------------------------------------------------------------===//
932+
933+
// When a ref is defined and used (non-discard) in a predecessor block, then
934+
// forwarded as a block arg via cond_br to a merge block, with a discard on
935+
// the other branch, the MOVE bit must be set on the block arg operand.
936+
//
937+
// Without the fix, isLastRealValueUse checked uses in ALL other blocks
938+
// (including predecessors) and found a "past" use, returning false and
939+
// preventing MOVE. This caused a ref leak: retain incremented the ref
940+
// count, but no discard existed on the taken path to release it.
941+
942+
// CHECK-LABEL: @module_predecessor_use_blockarg_move
943+
vm.module @module_predecessor_use_blockarg_move {
944+
945+
vm.import private @produce() -> !vm.buffer
946+
vm.import private @use_buffer(%buf : !vm.buffer)
947+
vm.import private @check_condition(%buf : !vm.buffer) -> i32
948+
949+
// CHECK-LABEL: @predecessor_use_blockarg_forward
950+
// Ref is used in the predecessor block, then forwarded as a block arg
951+
// via cond_br after a branch boundary. Mimics the while-loop ASAN leak.
952+
// MOVE must be set on the block arg operand.
953+
vm.func @predecessor_use_blockarg_forward() {
954+
%ref = vm.call @produce() : () -> !vm.buffer
955+
// Use %ref before the branch boundary.
956+
// CHECK: vm.call @use_buffer
957+
// CHECK-SAME: operand_registers = ["r0"]
958+
vm.call @use_buffer(%ref) : (!vm.buffer) -> ()
959+
%status = vm.call @check_condition(%ref) : (!vm.buffer) -> i32
960+
vm.br ^post_yield(%status : i32)
961+
962+
^post_yield(%result: i32):
963+
// Forward %ref as block arg on success, discard on error.
964+
// %ref is defined in the entry block (predecessor) and used there too.
965+
// MOVE must be set on the success branch's block arg operand.
966+
// CHECK: vm.cond_br
967+
// CHECK-SAME: operand_registers = ["i{{[0-9]+}}", "R0"]
968+
vm.cond_br %result, ^error, ^success(%ref : !vm.buffer)
969+
970+
^error:
971+
vm.discard.refs %ref : !vm.buffer
972+
vm.return
973+
974+
^success(%buf: !vm.buffer):
975+
vm.call @use_buffer(%buf) : (!vm.buffer) -> ()
976+
vm.discard.refs %buf : !vm.buffer
977+
vm.return
978+
}
979+
}

0 commit comments

Comments
 (0)