Skip to content

Commit e41cac3

Browse files
kshyattclaude
andcommitted
Don't push a poison aggregate onto the sret rooting worklist
`needsReRooting` walks every value stored into an sret argument to check each tracked pointer is also parked in the returnRoots array. Walking back through an insertvalue chain, the branch for an insert of an untracked field pushed the aggregate operand unconditionally: CountTrackedPointers tracked(IVI->getInsertedValueOperand()->getType()); if (tracked.count == 0) { storedValues.push_back(IVI->getAggregateOperand()); continue; } When that operand is the poison the chain was built from, the bare poison lands on the worklist. The struct handling below then looks for an extractvalue covering poison's tracked fields, finds none, and reports failed to find extracted pointer for %tape poison at index 1 before falling through to the pointer-shaped handling, whose assert(0) fires on any non-pointer value. Julia hits this whenever a nested sub-tape survives SROA as a whole-aggregate store and its first field is untracked -- reverse mode over PEPSKit's CTMRG aborts the process this way, since the shipped Enzyme_jll RelWithDebInfo build leaves assertions live. A poison, undef, or zeroinitializer aggregate has no live pointer in any field, so it needs no root regardless of which field the insertvalue overwrites. Hoist the trivial-base test above both branches and drop such a base, which is the reasoning the tracked branch three lines down already relies on. In the Julia case the tracked pointers are separately stored into the returnRoots, so the walk then completes and the pass correctly concludes it has no work to do. Also un-shadow `legal` in the struct handling. The inner declaration meant the coverage failure set a variable nobody read and fell into that same assert; report it through CustomErrorHandler like the pointer path does and reroot conservatively. This is not a behaviour change for NDEBUG builds, which already fell through to exactly that handler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2593327 commit e41cac3

2 files changed

Lines changed: 56 additions & 6 deletions

File tree

enzyme/Enzyme/FixupJuliaCallingConvention.cpp

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -223,15 +223,22 @@ bool needsReRooting(llvm::Argument *arg, bool &anyJLStore,
223223
}
224224
if (!foundUse) {
225225
if (auto IVI = dyn_cast<InsertValueInst>(sv)) {
226+
// An undef/poison/zeroinitializer base has no live pointer in any of
227+
// its fields, so it needs no root regardless of which field the
228+
// insertvalue overwrites.
229+
bool trivialAggregate =
230+
isa<UndefValue>(IVI->getAggregateOperand()) ||
231+
isa<PoisonValue>(IVI->getAggregateOperand()) ||
232+
isa<ConstantAggregateZero>(IVI->getAggregateOperand());
226233
CountTrackedPointers tracked(
227234
IVI->getInsertedValueOperand()->getType());
228235
if (tracked.count == 0) {
236+
if (trivialAggregate)
237+
continue;
229238
storedValues.push_back(IVI->getAggregateOperand());
230239
continue;
231240
}
232-
if (isa<UndefValue>(IVI->getAggregateOperand()) ||
233-
isa<PoisonValue>(IVI->getAggregateOperand()) ||
234-
isa<ConstantAggregateZero>(IVI->getAggregateOperand())) {
241+
if (trivialAggregate) {
235242
storedValues.push_back(IVI->getInsertedValueOperand());
236243
continue;
237244
}
@@ -240,7 +247,7 @@ bool needsReRooting(llvm::Argument *arg, bool &anyJLStore,
240247
continue;
241248
}
242249
if (auto ST = dyn_cast<StructType>(sv->getType())) {
243-
bool legal = true;
250+
bool covered = true;
244251
for (size_t i = 0; i < ST->getNumElements(); i++) {
245252

246253
CountTrackedPointers tracked(ST->getElementType(i));
@@ -336,13 +343,27 @@ bool needsReRooting(llvm::Argument *arg, bool &anyJLStore,
336343
if (!fullyCovered) {
337344
llvm::errs() << " failed to find extracted pointer for " << *sv
338345
<< " at index " << i << "\n";
339-
legal = false;
346+
covered = false;
340347
break;
341348
}
342349
}
343-
if (legal) {
350+
if (covered) {
344351
continue;
345352
}
353+
// Not every tracked pointer in this aggregate could be shown to be
354+
// rooted separately. Report it and reroot conservatively rather than
355+
// falling into the pointer-shaped handling below, which asserts on
356+
// any non-pointer value.
357+
if (hasReturnRootingAfterArg) {
358+
std::string s;
359+
llvm::raw_string_ostream ss(s);
360+
ss << "Could not find use of stored value\n";
361+
ss << " sv: " << *sv << "\n";
362+
CustomErrorHandler(ss.str().c_str(), wrap(sv), ErrorType::GCRewrite,
363+
nullptr, wrap(arg), nullptr);
364+
}
365+
legal = false;
366+
break;
346367
}
347368
if (!isa<PointerType>(sv->getType()) ||
348369
!isSpecialPtr(cast<PointerType>(sv->getType()))) {
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
; RUN: %opt %newLoadEnzyme -S -passes=enzyme-fixup-julia < %s | FileCheck %s
2+
3+
; A sub-aggregate rebuilt by an insertvalue chain based on poison, whose last
4+
; insert writes an untracked field. Walking back from the sret store reaches that
5+
; insertvalue, and pushing its aggregate operand puts the bare poison on the
6+
; worklist; the struct handling then looks for an extractvalue covering poison's
7+
; tracked field, finds none, and used to fall through to an assert. A poison
8+
; aggregate holds no live pointer, so there is nothing to root and the pass has
9+
; no work to do here -- the tracked pointer is already in the returnRoots.
10+
11+
; CHECK-LABEL: define void @test_poison_insertvalue_base({{.*}} sret({{.*}}) %sret, {{.*}}"enzymejl_returnRoots"="1" %rroots
12+
; CHECK: store {{.*}} %o, {{.*}} %sret
13+
; CHECK: ret void
14+
15+
%tape = type { i8*, {} addrspace(10)*, i64 }
16+
%outer = type { %tape }
17+
18+
define void @test_poison_insertvalue_base(%outer* sret(%outer) %sret, [1 x {} addrspace(10)*]* "enzymejl_returnRoots"="1" %rroots, i8* %raw, {} addrspace(10)* %tracked, i64 %n) {
19+
entry:
20+
%t0 = insertvalue %tape poison, i8* %raw, 0
21+
%t1 = insertvalue %tape %t0, {} addrspace(10)* %tracked, 1
22+
%t2 = insertvalue %tape %t1, i64 %n, 2
23+
%o = insertvalue %outer poison, %tape %t2, 0
24+
store %outer %o, %outer* %sret, align 8
25+
26+
%g = getelementptr inbounds [1 x {} addrspace(10)*], [1 x {} addrspace(10)*]* %rroots, i64 0, i64 0
27+
store {} addrspace(10)* %tracked, {} addrspace(10)** %g, align 8
28+
ret void
29+
}

0 commit comments

Comments
 (0)