Repository navigation
[SCEV] Drop stale no-wrap flags from hoisted IV increments - #9121
Natalie Chouinard (sudonatalie) wants to merge 1 commit into
Conversation
replaceCongruentIVs could hoist an IV increment above a loop exit guard while keeping nsw/nuw flags that only held below it, letting InstCombine remove an array index mask. Drop those flags, keeping only flags shared by both increments, and only apply an increment's flags to the post-increment add rec when overflow would be UB. Partial backports of several upstream LLVM changes.
There was a problem hiding this comment.
🟡 Changes recommended
The latch-reachability check relies on mayThrow(), allowing nounwind calls that terminate or never return to cause an earlier wrapping increment to be incorrectly marked nsw.
1 open issue
What changed in this PR
This PR partially backports upstream LLVM fixes to prevent stale no-wrap flags from causing incorrect loop optimizations in DXC, including removal of array-index masks.
Changes:
- Drops poison-generating flags when hoisting or reusing induction-variable increments, retaining compatible shared no-wrap flags.
- Restricts propagation of increment flags to post-increment recurrences using a poison-safety check.
- Adds regression tests for guarded increments, dominating increments, pointer arithmetic, and flag preservation.
| File | Description |
|---|---|
tools/clang/test/DXC/Passes/ScalarEvolution/post-inc-nowrap-flags.ll |
Tests when post-increment recurrences may inherit no-wrap flags. |
tools/clang/test/DXC/Passes/IndVarSimplify/iv-hoist-poison-flags.ll |
Tests flag removal and preservation when hoisting or reusing increments. |
tools/clang/test/DXC/Passes/IndVarSimplify/guarded-latch-inc-flags.ll |
Checks that guarded increment flags do not affect earlier wrapping additions. |
lib/Analysis/ScalarEvolutionExpander.cpp |
Drops unsafe increment flags and preserves compatible shared flags. |
lib/Analysis/ScalarEvolution.cpp |
Adds poison-safety analysis and cached loop exception checks. |
include/llvm/Analysis/ScalarEvolutionExpander.h |
Adds the flag-dropping option to increment hoisting. |
include/llvm/Analysis/ScalarEvolution.h |
Declares the poison-safety helper and loop exception cache. |
Review effort: Balanced
| MayThrow = std::any_of(BB->begin(), BB->end(), | ||
| [](Instruction &I) { return I.mayThrow(); }); |
Steven Perron (s-perron)
left a comment
Steven Perron (s-perron)
left a comment
There was a problem hiding this comment.
I see that you have a lot of different backports. I found it hard to follow what you changed from the original backport, and what is a change from a later one.
Is it possible to have them split up into multiple commits, so that I can see the evolution more clearly.
I'll take another look at it on Monday.
| // isAddRecNeverPoison is more conservative than upstream's. | ||
|
|
||
| // We can add Flags to the post-inc expression only if we | ||
| // know that it us *undefined behavior* for BEValueV to |
There was a problem hiding this comment.
| // know that it us *undefined behavior* for BEValueV to | |
| // know that it is *undefined behavior* for BEValueV to |
If this is the same a upstream, I'm fine not accepting for consistency.
| ConstantEvolutionLoopExitValue.clear(); | ||
| ValuesAtScopes.clear(); | ||
| LoopDispositions.clear(); | ||
| LoopMayThrow.clear(); // HLSL Change - Partial backport of PR27315. |
There was a problem hiding this comment.
I don't see this in the llvm commit. I don't think it is bad, but the comment might need updating.
| /// Drop the poison-generating flags (nuw, nsw, exact and inbounds) of \p I. | ||
| /// LLVM 3.7 lacks Instruction::dropPoisonGeneratingFlags. | ||
| static void dropPoisonGeneratingFlags(Instruction *I) { |
There was a problem hiding this comment.
Is this just the code that is in Instruction? If so, why don't we just add the member function in case it is needed for another backport?
replaceCongruentIVs could hoist an IV increment above a loop exit guard while keeping nsw/nuw flags that only held below it, letting InstCombine remove an array index mask. Drop those flags, keeping only flags shared by both increments, and only apply an increment's flags to the post-increment add rec when overflow would be UB.
Partial backports of several upstream LLVM changes.