[LIR][SCEVExpander] Restore original flags when aborting transform #82362
Conversation
This file contains 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
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesSCEVExpanderCleaner will currently remove instructions created by SCEVExpander, but not restore poison generating flags that it may have dropped. As such, running LIR can currently spuriously drop flags without performing any transforms. Fix this by keeping track of original instruction flags in SCEVExpander. Fixes #82337. Full diff: https://github.com/llvm/llvm-project/pull/82362.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
index 035705b7f4b78b..955e6d967088ac 100644
--- a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
+++ b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
@@ -41,6 +41,17 @@ struct SCEVOperand {
const SCEV* S;
};
+struct PoisonFlags {
+ unsigned NUW : 1;
+ unsigned NSW : 1;
+ unsigned Exact : 1;
+ unsigned Disjoint : 1;
+ unsigned NNeg : 1;
+
+ PoisonFlags(const Instruction *I);
+ void apply(Instruction *I);
+};
+
/// This class uses information about analyze scalars to rewrite expressions
/// in canonical form.
///
@@ -48,6 +59,8 @@ struct SCEVOperand {
/// and destroy it when finished to allow the release of the associated
/// memory.
class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
+ friend class SCEVExpanderCleaner;
+
ScalarEvolution &SE;
const DataLayout &DL;
@@ -70,6 +83,10 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
/// InsertedValues/InsertedPostIncValues.
SmallPtrSet<Value *, 16> ReusedValues;
+ /// Original flags of instructions for which they were modified. Used
+ /// by SCEVExpanderCleaner to undo changes.
+ DenseMap<AssertingVH<Instruction>, PoisonFlags> OrigFlags;
+
// The induction variables generated.
SmallVector<WeakVH, 2> InsertedIVs;
@@ -188,6 +205,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
InsertedValues.clear();
InsertedPostIncValues.clear();
ReusedValues.clear();
+ OrigFlags.clear();
ChainedPhis.clear();
InsertedIVs.clear();
}
@@ -483,6 +501,8 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
void rememberInstruction(Value *I);
+ void rememberFlags(Instruction *I);
+
bool isNormalAddRecExprPHI(PHINode *PN, Instruction *IncV, const Loop *L);
bool isExpandedAddRecExprPHI(PHINode *PN, Instruction *IncV, const Loop *L);
diff --git a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
index 3a28909473d95f..2e6e5c310faab7 100644
--- a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -43,6 +43,37 @@ cl::opt<unsigned> llvm::SCEVCheapExpansionBudget(
using namespace PatternMatch;
+PoisonFlags::PoisonFlags(const Instruction *I) {
+ NUW = false;
+ NSW = false;
+ Exact = false;
+ Disjoint = false;
+ NNeg = false;
+ if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I)) {
+ NUW = OBO->hasNoUnsignedWrap();
+ NSW = OBO->hasNoUnsignedWrap();
+ }
+ if (auto *PEO = dyn_cast<PossiblyExactOperator>(I))
+ Exact = PEO->isExact();
+ if (auto *PDI = dyn_cast<PossiblyDisjointInst>(I))
+ Disjoint = PDI->isDisjoint();
+ if (auto *PNI = dyn_cast<PossiblyNonNegInst>(I))
+ NNeg = PNI->hasNonNeg();
+}
+
+void PoisonFlags::apply(Instruction *I) {
+ if (isa<OverflowingBinaryOperator>(I)) {
+ I->setHasNoUnsignedWrap(NUW);
+ I->setHasNoUnsignedWrap(NSW);
+ }
+ if (isa<PossiblyExactOperator>(I))
+ I->setIsExact(Exact);
+ if (auto *PDI = dyn_cast<PossiblyDisjointInst>(I))
+ PDI->setIsDisjoint(Disjoint);
+ if (auto *PNI = dyn_cast<PossiblyNonNegInst>(I))
+ PNI->setNonNeg(NNeg);
+}
+
/// ReuseOrCreateCast - Arrange for there to be a cast of V to Ty at IP,
/// reusing an existing cast if a suitable one (= dominating IP) exists, or
/// creating a new one.
@@ -724,6 +755,7 @@ bool SCEVExpander::hoistIVInc(Instruction *IncV, Instruction *InsertPos,
auto FixupPoisonFlags = [this](Instruction *I) {
// Drop flags that are potentially inferred from old context and infer flags
// in new context.
+ rememberFlags(I);
I->dropPoisonGeneratingFlags();
if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I))
if (auto Flags = SE.getStrengthenedNoWrapFlagsFromBinOp(OBO)) {
@@ -1472,6 +1504,7 @@ Value *SCEVExpander::expand(const SCEV *S) {
V = fixupLCSSAFormFor(V);
} else {
for (Instruction *I : DropPoisonGeneratingInsts) {
+ rememberFlags(I);
I->dropPoisonGeneratingFlagsAndMetadata();
// See if we can re-infer from first principles any of the flags we just
// dropped.
@@ -1512,6 +1545,11 @@ void SCEVExpander::rememberInstruction(Value *I) {
DoInsert(I);
}
+void SCEVExpander::rememberFlags(Instruction *I) {
+ // If we already have flags for the instruction, keep the existing ones.
+ OrigFlags.try_emplace(I, PoisonFlags(I));
+}
+
void SCEVExpander::replaceCongruentIVInc(
PHINode *&Phi, PHINode *&OrigPhi, Loop *L, const DominatorTree *DT,
SmallVectorImpl<WeakTrackingVH> &DeadInsts) {
@@ -2309,6 +2347,10 @@ void SCEVExpanderCleaner::cleanup() {
if (ResultUsed)
return;
+ // Restore original poison flags.
+ for (auto [I, Flags] : Expander.OrigFlags)
+ Flags.apply(I);
+
auto InsertedInstructions = Expander.getAllInsertedInstructions();
#ifndef NDEBUG
SmallPtrSet<Instruction *, 8> InsertedSet(InsertedInstructions.begin(),
diff --git a/llvm/test/Transforms/LoopIdiom/pr82337.ll b/llvm/test/Transforms/LoopIdiom/pr82337.ll
index e8a6e1704f7c17..da9eb14af3f0a4 100644
--- a/llvm/test/Transforms/LoopIdiom/pr82337.ll
+++ b/llvm/test/Transforms/LoopIdiom/pr82337.ll
@@ -1,15 +1,15 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt -S -passes=loop-idiom < %s | FileCheck %s
-; FIXME: The poison flags should be preserved, as no transform takes place.
+; The poison flags should be preserved, as no transform takes place.
define void @test(ptr %p.end, ptr %p.start) {
; CHECK-LABEL: define void @test(
; CHECK-SAME: ptr [[P_END:%.*]], ptr [[P_START:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[P_END_INT:%.*]] = ptrtoint ptr [[P_END]] to i64
; CHECK-NEXT: [[P_START_INT:%.*]] = ptrtoint ptr [[P_START]] to i64
-; CHECK-NEXT: [[DIST:%.*]] = sub i64 [[P_END_INT]], [[P_START_INT]]
-; CHECK-NEXT: [[LEN:%.*]] = lshr i64 [[DIST]], 5
+; CHECK-NEXT: [[DIST:%.*]] = sub nuw i64 [[P_END_INT]], [[P_START_INT]]
+; CHECK-NEXT: [[LEN:%.*]] = lshr exact i64 [[DIST]], 5
; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr [[P_END]], [[P_START]]
; CHECK-NEXT: br i1 [[CMP]], label [[EXIT:%.*]], label [[PREHEADER:%.*]]
; CHECK: preheader:
|
| NNeg = false; | ||
| if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I)) { | ||
| NUW = OBO->hasNoUnsignedWrap(); | ||
| NSW = OBO->hasNoUnsignedWrap(); |
There was a problem hiding this comment.
Shouldn't this be hasNoSignedWrap? Same as below when setting.
There was a problem hiding this comment.
Indeed! Thanks for catching it.
SCEVExpanderCleaner will currently remove instructions created by SCEVExpander, but not restore poison generating flags that it may have dropped. As such, running LIR can currently spuriously drop flags without performing any transforms. Fix this by keeping track of original instruction flags in SCEVExpander. Fixes llvm#82337.
nikic
force-pushed
the
lir-cleanup-fix
branch
from
ac18c80
to
43d0a00
Compare
|
This patch causes a crash when building the Linux kernel for RISC-V. A trivial C reproducer from struct altera_config {
char action;
} altera_execute_astate;
char *altera_execute_p;
unsigned altera_execute_pc;
int altera_execute_args[3];
int altera_execute_arg_count, altera_execute_i;
void _printk(char *, ...);
int altera_execute() {
struct altera_config aconf = altera_execute_astate;
int opcode;
if (aconf.action) {
opcode = altera_execute_p[altera_execute_pc];
_printk("", opcode);
altera_execute_arg_count = opcode >> 6;
for (altera_execute_i = 0; altera_execute_i < altera_execute_arg_count;
++altera_execute_i) {
altera_execute_args[altera_execute_i] =
altera_execute_p[altera_execute_pc];
altera_execute_pc += 4;
}
}
return 4;
} |
|
@nathanchance Thanks for the report! Here's a reduced test case: ; RUN: opt -S -passes=loop-reduce < %s
target triple = "riscv64-unknown-linux-gnu"
define void @test(ptr %p, i8 %arg, i32 %start) {
entry:
%conv = zext i8 %arg to i32
%shr = lshr i32 %conv, 1
%wide.trip.count = zext nneg i32 %shr to i64
br label %for.body
for.body:
%indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
%add810 = phi i32 [ %start, %entry ], [ %add, %for.body ]
%idxprom2 = zext i32 %add810 to i64
%arrayidx3 = getelementptr i8, ptr %p, i64 %idxprom2
%v = load i8, ptr %arrayidx3, align 1
%add = add i32 %add810, 1
%indvars.iv.next = add i64 %indvars.iv, 1
%exitcond.not = icmp eq i64 %indvars.iv, %wide.trip.count
br i1 %exitcond.not, label %exit, label %for.body
exit:
ret void
} |
nikic
added a commit
that referenced
this pull request
Feb 22, 2024
To avoid an assertion failure when an AssertingVH is removed, as reported in: #82362 (comment) Also remove an unnecessary use of SCEVExpanderCleaner.
|
The issue should be fixed by 7276352. |
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.
SCEVExpanderCleaner will currently remove instructions created by SCEVExpander, but not restore poison generating flags that it may have dropped. As such, running LIR can currently spuriously drop flags without performing any transforms.
Fix this by keeping track of original instruction flags in SCEVExpander.
Fixes #82337.