[Local] Move OverflowTracking to Local.h, move logic to helpers (NFC) #140403
Conversation
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
|
@llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesMove parts of the logic used by Reassociate to OverflowTracking (mergeFlags & applyFlags) and move the definition to Local.h (not sure if there's a better place?). For now it just moves the NUW/NSW handling, as this matches the uses in LICM. I'll look into the FP math handling separately, as it looks like there's a difference between Reassociate (takes all flags from I, while LICM takes the intersection of the flags on both instructions). Full diff: https://github.com/llvm/llvm-project/pull/140403.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Scalar/Reassociate.h b/llvm/include/llvm/Transforms/Scalar/Reassociate.h
index 23b70164d96a4..a5d137661e11e 100644
--- a/llvm/include/llvm/Transforms/Scalar/Reassociate.h
+++ b/llvm/include/llvm/Transforms/Scalar/Reassociate.h
@@ -39,6 +39,7 @@ class Function;
class Instruction;
class IRBuilderBase;
class Value;
+struct OverflowTracking;
/// A private "module" namespace for types and utilities used by Reassociate.
/// These are implementation details and should not be used by clients.
@@ -64,17 +65,6 @@ struct Factor {
Factor(Value *Base, unsigned Power) : Base(Base), Power(Power) {}
};
-struct OverflowTracking {
- bool HasNUW = true;
- bool HasNSW = true;
- bool AllKnownNonNegative = true;
- bool AllKnownNonZero = true;
- // Note: AllKnownNonNegative can be true in a case where one of the operands
- // is negative, but one the operators is not NSW. AllKnownNonNegative should
- // not be used independently of HasNSW
- OverflowTracking() = default;
-};
-
class XorOpnd;
} // end namespace reassociate
@@ -115,7 +105,7 @@ class ReassociatePass : public PassInfoMixin<ReassociatePass> {
void ReassociateExpression(BinaryOperator *I);
void RewriteExprTree(BinaryOperator *I,
SmallVectorImpl<reassociate::ValueEntry> &Ops,
- reassociate::OverflowTracking Flags);
+ OverflowTracking Flags);
Value *OptimizeExpression(BinaryOperator *I,
SmallVectorImpl<reassociate::ValueEntry> &Ops);
Value *OptimizeAdd(Instruction *I,
diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index db064e1f41f02..d57e741aba206 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -556,6 +556,27 @@ Value *invertCondition(Value *Condition);
/// function, explicitly materialize the maximal set in the IR.
bool inferAttributesFromOthers(Function &F);
+//===----------------------------------------------------------------------===//
+// Helpers to track and update flags on instructions.
+//
+
+struct OverflowTracking {
+ bool HasNUW = true;
+ bool HasNSW = true;
+ bool AllKnownNonNegative = true;
+ bool AllKnownNonZero = true;
+ // Note: AllKnownNonNegative can be true in a case where one of the operands
+ // is negative, but one the operators is not NSW. AllKnownNonNegative should
+ // not be used independently of HasNSW
+ OverflowTracking() = default;
+
+ /// Merge in the no-wrap flags from \p I.
+ void mergeFlags(Instruction &I);
+
+ /// Apply the no-wrap flags to \p I if applicable.
+ void applyFlags(Instruction &I);
+};
+
} // end namespace llvm
#endif // LLVM_TRANSFORMS_UTILS_LOCAL_H
diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index cb7a9ef9b6711..778a6a012556b 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -382,7 +382,7 @@ using RepeatedValue = std::pair<Value *, uint64_t>;
static bool LinearizeExprTree(Instruction *I,
SmallVectorImpl<RepeatedValue> &Ops,
ReassociatePass::OrderedSet &ToRedo,
- reassociate::OverflowTracking &Flags) {
+ OverflowTracking &Flags) {
assert((isa<UnaryOperator>(I) || isa<BinaryOperator>(I)) &&
"Expected a UnaryOperator or BinaryOperator!");
LLVM_DEBUG(dbgs() << "LINEARIZE: " << *I << '\n');
@@ -431,10 +431,7 @@ static bool LinearizeExprTree(Instruction *I,
// We examine the operands of this binary operator.
auto [I, Weight] = Worklist.pop_back_val();
- if (isa<OverflowingBinaryOperator>(I)) {
- Flags.HasNUW &= I->hasNoUnsignedWrap();
- Flags.HasNSW &= I->hasNoSignedWrap();
- }
+ Flags.mergeFlags(*I);
for (unsigned OpIdx = 0; OpIdx < I->getNumOperands(); ++OpIdx) { // Visit operands.
Value *Op = I->getOperand(OpIdx);
@@ -734,15 +731,7 @@ void ReassociatePass::RewriteExprTree(BinaryOperator *I,
ExpressionChangedStart->clearSubclassOptionalData();
ExpressionChangedStart->setFastMathFlags(Flags);
} else {
- ExpressionChangedStart->clearSubclassOptionalData();
- if (ExpressionChangedStart->getOpcode() == Instruction::Add ||
- (ExpressionChangedStart->getOpcode() == Instruction::Mul &&
- Flags.AllKnownNonZero)) {
- if (Flags.HasNUW)
- ExpressionChangedStart->setHasNoUnsignedWrap();
- if (Flags.HasNSW && (Flags.AllKnownNonNegative || Flags.HasNUW))
- ExpressionChangedStart->setHasNoSignedWrap();
- }
+ Flags.applyFlags(*ExpressionChangedStart);
}
}
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 3dbd605e19c3a..4d168ce7cf591 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -4362,3 +4362,21 @@ bool llvm::inferAttributesFromOthers(Function &F) {
return Changed;
}
+
+void OverflowTracking::mergeFlags(Instruction &I) {
+ if (isa<OverflowingBinaryOperator>(&I)) {
+ HasNUW &= I.hasNoUnsignedWrap();
+ HasNSW &= I.hasNoSignedWrap();
+ }
+}
+
+void OverflowTracking::applyFlags(Instruction &I) {
+ I.clearSubclassOptionalData();
+ if (I.getOpcode() == Instruction::Add ||
+ (I.getOpcode() == Instruction::Mul && AllKnownNonZero)) {
+ if (HasNUW)
+ I.setHasNoUnsignedWrap();
+ if (HasNSW && (AllKnownNonNegative || HasNUW))
+ I.setHasNoSignedWrap();
+ }
+}
|
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
May 17, 2025
This enables preserving NSW when both adds have NSW and NUW. For now, set AllKnownNonNegative/AllKnownNonZero to false when using in LICM. https://alive2.llvm.org/ce/z/uu79Xc Depends on llvm#140403 (included in PR)
preames
approved these changes
May 18, 2025
dtcxzyw
approved these changes
May 18, 2025
| struct OverflowTracking { | ||
| bool HasNUW = true; | ||
| bool HasNSW = true; | ||
| bool AllKnownNonNegative = true; |
There was a problem hiding this comment.
Please add a comment indicating that the user is responsible for updating AllKnownNonNegative and AllKnownNonZero.
preames
reviewed
May 18, 2025
| OverflowTracking() = default; | ||
|
|
||
| /// Merge in the no-wrap flags from \p I. | ||
| void mergeFlags(Instruction &I); |
There was a problem hiding this comment.
Random thought - maybe we should have two merge routines, one for the interior nodes being reassociated, and one for the leaves? This would let us move all the KnownNonNegative and KnownNonZero logic out, and reuse it from LICM.
There was a problem hiding this comment.
Will share a separate patch for that, thanks
Move parts of the logic used by Reassociate to OverflowTracking (mergeFlags & applyFlags) and move the definition to Local.h (not sure if there's a better place?). For now it just moves the NUW/NSW handling, as this matches the uses in LICM. I'll look into the FP math handling separately, as it looks like there's a difference between Reassociate (takes all flags from I, while LICM takes the intersection of the flags on both instructions).
fhahn
force-pushed
the
generalize-overflow-tracking
branch
from
545b4cd to
6902ceb
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/15462 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 19, 2025
…lpers (NFC) (#140403) Move parts of the logic used by Reassociate to OverflowTracking (mergeFlags & applyFlags) and move the definition to Local.h. For now it just moves the NUW/NSW handling, as this matches the uses in LICM. I'll look into the FP math handling separately, as it looks like there's a difference between Reassociate (takes all flags from I, while LICM takes the intersection of the flags on both instructions). PR: llvm/llvm-project#140403
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
May 20, 2025
This enables preserving NSW when both adds have NSW and NUW. For now, set AllKnownNonNegative/AllKnownNonZero to false when using in LICM. https://alive2.llvm.org/ce/z/uu79Xc Depends on llvm#140403 (included in PR)
fhahn
added a commit
that referenced
this pull request
May 20, 2025
…140404) This enables preserving NSW when both adds have NSW and NUW. For now, set AllKnownNonNegative/AllKnownNonZero to false when using in LICM. https://alive2.llvm.org/ce/z/uu79Xc Depends on #140403. PR: #140404
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 20, 2025
…ociating. (#140404) This enables preserving NSW when both adds have NSW and NUW. For now, set AllKnownNonNegative/AllKnownNonZero to false when using in LICM. https://alive2.llvm.org/ce/z/uu79Xc Depends on llvm/llvm-project#140403. PR: llvm/llvm-project#140404
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…llvm#140403) Move parts of the logic used by Reassociate to OverflowTracking (mergeFlags & applyFlags) and move the definition to Local.h. For now it just moves the NUW/NSW handling, as this matches the uses in LICM. I'll look into the FP math handling separately, as it looks like there's a difference between Reassociate (takes all flags from I, while LICM takes the intersection of the flags on both instructions). PR: llvm#140403
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…lvm#140404) This enables preserving NSW when both adds have NSW and NUW. For now, set AllKnownNonNegative/AllKnownNonZero to false when using in LICM. https://alive2.llvm.org/ce/z/uu79Xc Depends on llvm#140403. PR: llvm#140404
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.
Move parts of the logic used by Reassociate to OverflowTracking (mergeFlags & applyFlags) and move the definition to Local.h (not sure if there's a better place?).
For now it just moves the NUW/NSW handling, as this matches the uses in LICM. I'll look into the FP math handling separately, as it looks like there's a difference between Reassociate (takes all flags from I, while LICM takes the intersection of the flags on both instructions).