FixAddrSpaceCast: place lowered PHI loads on their incoming edge - #426
Open
pvelesko wants to merge 2 commits into
Open
FixAddrSpaceCast: place lowered PHI loads on their incoming edge#426pvelesko wants to merge 2 commits into
pvelesko wants to merge 2 commits into
Conversation
removeAddrSpaceCastsFromPhiIncomingBBs() replaces a load from a PHI of generic pointers with one load per incoming edge. Each of those loads has to execute on the edge it belongs to. Add a test with the shape that breaks it: a 3-edge PHI inside a loop whose incoming values come from mixed storage classes (two Workgroup, one Function). The Function edge's pointer is defined by an addrspacecast in the entry block, so anchoring the new load at that definition hoists it out of the loop entirely, where it reads the private accumulator before it has been initialised. A second case covers a load hoisted above a store to the same location. Expected to fail until the placement is corrected. Reduced from intel#398. Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
lowerLoadsfromPHI() rewrites a load from a PHI of generic pointers into one load per incoming edge, but built its IRBuilder at the definition of the incoming pointer (IRBuilder<> Builder(LoadSource), NewInst->insertAfter( LoadSource)). That is only correct when the definition happens to be the last thing on the edge. It frequently is not. When the incoming value is an addrspacecast of an alloca, the cast is hoisted to the entry block or a loop preheader, so the per-edge load is emitted there too, outside the loop containing the PHI. An accumulator is then read once, before it is ever initialised, and every iteration merges that stale value. The same mistake hoists the load above stores to the location it reads. Build one IRBuilder per incoming edge at the incoming block's terminator and insert both the bitcast and the load through it. SSA guarantees the incoming value dominates that point, so the insertion is always legal, and the load can only move later than before, never earlier. This also removes a latent segfault. Under opaque pointers the CreateBitCast above is a no-op cast, so it returns its operand rather than a new instruction; the following cast<Instruction> is unchecked in a Release build, so LoadSource became a bogus Instruction* whenever the addrspacecast operand was an Argument or a global. insertAfter() then dereferenced it and crashed. Builder.Insert() never dereferences LoadSource, and the remaining setOperand(0, LoadSource) is pointer-identical to the operand's Value*, so it stays correct. lowerLoadsDifferentIncomingAndParent.ll had the old placement baked into its CHECK lines: it expected the bitcast and load in the addrspacecast's parent block (%cond.false3459) rather than in the incoming block (%cond.between). Updated to the correct block. That file is UNSUPPORTED: llvm-17-plus, so it only runs on LLVM 16 and older; leaving its CHECK lines alone would have left it failing there, because the fix definitively moves those instructions out of %cond.false3459. Fixes the test added in the preceding commit. Fixes intel#398 Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
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
1 participant
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.
lowerLoadsfromPHI()anchors each per-edge load at the definition of the incoming pointer (FixAddrSpaceCast.cpp:136and:159) instead of in the PHI's incoming block. The definition only has to dominate the incoming block, so the load can be emitted in an earlier block, ahead of stores the original load was ordered after.Input:
The store writes through
%addr.res, which is%prelabelled, so%vhas to read what the store wrote.Before, the lowered load is hoisted above that store and reads stale memory:
false.bb: - %1 = load i32, ptr addrspace(3) %p, align 4 %addr.res = addrspacecast ptr addrspace(3) %p to ptr addrspace(4) store i32 0, ptr addrspace(4) %addr.res, align 4 br label %mergeAfter:
false.bb: %addr.res = addrspacecast ptr addrspace(3) %p to ptr addrspace(4) store i32 0, ptr addrspace(4) %addr.res, align 4 + %1 = load i32, ptr addrspace(3) %p, align 4 br label %mergeInserting at the incoming block's terminator is always legal: SSA guarantees the incoming value dominates it.
lowerLoadsDifferentIncomingAndParent.llis updated because it covers exactly this shape.%addr.resis defined in%cond.false3459while the PHI's incoming block is%cond.between, and the load now lands in the latter. That file isUNSUPPORTED: llvm-17-plus, so I could not execute it here; I ran the same function in opaque-pointer form instead and the load moves from%cond.false3459to%cond.betweenas the updated CHECK lines expect.Fixes #398