Fix unsound icmp fold for loads from fresh allocations in jl-inst-simplify#2919
Merged
Conversation
…plify arePointersGuaranteedNoAlias treated a pointer loaded from a fresh allocation as unequal to any other pointer, scanning for clobbering writes only between the load and the compare. That interval is wrong: the loaded SSA value is fixed at load time, and a store INTO the allocation before the load (e.g. julia lowering a captured, reassigned local into a Core.Box) makes the loaded value an arbitrary captured pointer. jl-inst-simplify then folded `x !== nothing` to true for x === nothing whenever x round-tripped through a Core.Box, e.g. in Enzyme.jl's own lower_convention when compiled into a differentiated module via nested_codegen! of a custom rule (EnzymeAD/Enzyme.jl CUDA "Reverse Kernel" CI failure: AssertionError: codegen_i=2 > length(codegen_types)=1 ...). Scan for clobbers between the allocation and the load instead: if nothing wrote to the loaded location in that interval, the loaded value is undef or an allocator-installed fresh pointer and the fold is legal; otherwise bail. All existing JLSimplify tests produce identical output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
arePointersGuaranteedNoAlias(Utils.cpp) treats a pointer loaded from a fresh allocation as guaranteed-unequal to another pointer, scanning for clobbering writes only between the load and the compare. That interval is wrong: the loaded SSA value is fixed at load time, so the scan proves nothing. If a store into the allocation happens before the load, the loaded value is an arbitrary captured pointer — including the compared-against one.jl-inst-simplifythen folds theicmpto a constant.Minimal shape (folds to
falsebefore this patch, even though it is alwaystrue):Julia hits this whenever a value round-trips through a
Core.Box(captured + reassigned local):x !== nothingfolds totrueforx === nothing.How it surfaced
Enzyme.jl's CUDA CI ("Reverse Kernel" testset) fails with
CUDACore's
augmented_primalrule callsEnzyme.tape_typeat runtime;nested_codegen!compiles the rule's whole call graph — includingEnzyme.Compiler.lower_convention— into the differentiated module, where this fold rewritesreturnRoots = returnRoots !== nothinginto an unconditionalstore jl_true(returnRootsis aCore.Boxthere).classify_argumentsthen seeshas_returnroots=truefor aNothing-returning kernel and asserts.The fix
Scan for clobbers between the allocation and the load instead. If nothing wrote to the loaded location in that interval, the loaded value is
undefor an allocator-installed fresh pointer, and the fold remains legal; otherwise bail. The previously unguardedisa<Argument>(end)fast path had the same hole and now sits behind the same check.Testing
enzyme/test/Enzyme/JLSimplify/boxedroundtrip.ll: the boxed roundtrip compare must survive; the no-store variant must still fold. Fails against the unpatched pass, passes with this patch.opt); in particularloads.ll's intended fresh-array-data fold still fires.deps/build_local.jl: a CPU-only reproducer (custom rule shape with aCore.BoxedUnion{Nothing,Type}local) errors on the released jll and computes the correct gradient with this patch; the CUDA "Reverse Kernel" assertion above no longer triggers.🤖 Generated with Claude Code