Skip to content

[Local] Verify opcodes match for all insts passed to mergeFlags (NFC). #141231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 28, 2025

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented May 23, 2025

The logic for tracking flags relies on all instructions having the same opcode. Add an assert to check, as suggested in #140406.

@llvmbot
Copy link
Member

llvmbot commented May 23, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

The logic for tracking flags relies on all instructions having the same opcode. Add an assert to check, as suggested in #140406.


Full diff: https://github.com/llvm/llvm-project/pull/141231.diff

4 Files Affected:

  • (modified) llvm/include/llvm/Transforms/Utils/Local.h (+7)
  • (modified) llvm/lib/Transforms/Scalar/LICM.cpp (-7)
  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+11)
  • (modified) llvm/test/Transforms/Reassociate/or-disjoint.ll (+2-2)
diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index fa26446f9492a..6162557496405 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -563,6 +563,13 @@ bool inferAttributesFromOthers(Function &F);
 struct OverflowTracking {
   bool HasNUW = true;
   bool HasNSW = true;
+  bool IsDisjoint = true;
+
+#ifndef NDEBUG
+  /// Opcode of merged instructions. All instructions passed to mergeFlags must
+  /// have the same opcode.
+  std::optional<unsigned> Opcode;
+#endif
 
   // Note: At the moment, users are responsible to manage AllKnownNonNegative
   // and AllKnownNonZero manually. AllKnownNonNegative can be true in a case
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 7c7e0dcff0886..006a09b38bc71 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -2870,13 +2870,6 @@ static bool hoistBOAssociation(Instruction &I, Loop &L,
     if (auto *I = dyn_cast<Instruction>(Inv))
       I->setFastMathFlags(Intersect);
     NewBO->setFastMathFlags(Intersect);
-  } else if (Opcode == Instruction::Or) {
-    bool Disjoint = cast<PossiblyDisjointInst>(BO)->isDisjoint() &&
-                    cast<PossiblyDisjointInst>(BO0)->isDisjoint();
-    // If `Inv` was not constant-folded, a new Instruction has been created.
-    if (auto *I = dyn_cast<PossiblyDisjointInst>(Inv))
-      I->setIsDisjoint(Disjoint);
-    cast<PossiblyDisjointInst>(NewBO)->setIsDisjoint(Disjoint);
   } else {
     OverflowTracking Flags;
     Flags.AllKnownNonNegative = false;
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 4d168ce7cf591..fe1391a501b43 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -4364,10 +4364,19 @@ bool llvm::inferAttributesFromOthers(Function &F) {
 }
 
 void OverflowTracking::mergeFlags(Instruction &I) {
+#ifndef NDEBUG
+  if (Opcode)
+    assert(Opcode == I.getOpcode() &&
+           "can only use mergeFlags on instructions with matching opcodes");
+  else
+    Opcode = I.getOpcode();
+#endif
   if (isa<OverflowingBinaryOperator>(&I)) {
     HasNUW &= I.hasNoUnsignedWrap();
     HasNSW &= I.hasNoSignedWrap();
   }
+  if (auto *DisjointOp = dyn_cast<PossiblyDisjointInst>(&I))
+    IsDisjoint &= DisjointOp->isDisjoint();
 }
 
 void OverflowTracking::applyFlags(Instruction &I) {
@@ -4379,4 +4388,6 @@ void OverflowTracking::applyFlags(Instruction &I) {
     if (HasNSW && (AllKnownNonNegative || HasNUW))
       I.setHasNoSignedWrap();
   }
+  if (auto *DisjointOp = dyn_cast<PossiblyDisjointInst>(&I))
+    DisjointOp->setIsDisjoint(IsDisjoint);
 }
diff --git a/llvm/test/Transforms/Reassociate/or-disjoint.ll b/llvm/test/Transforms/Reassociate/or-disjoint.ll
index 777836ed98152..b060b94e01d69 100644
--- a/llvm/test/Transforms/Reassociate/or-disjoint.ll
+++ b/llvm/test/Transforms/Reassociate/or-disjoint.ll
@@ -4,8 +4,8 @@
 
 define i16 @or_disjoint_both(i16 %a, i16 %b) {
 ; CHECK-LABEL: @or_disjoint_both(
-; CHECK-NEXT:    [[OR_1:%.*]] = or i16 [[A:%.*]], 1
-; CHECK-NEXT:    [[OR_2:%.*]] = or i16 [[OR_1]], [[B:%.*]]
+; CHECK-NEXT:    [[OR_1:%.*]] = or disjoint i16 [[A:%.*]], 1
+; CHECK-NEXT:    [[OR_2:%.*]] = or disjoint i16 [[OR_1]], [[B:%.*]]
 ; CHECK-NEXT:    ret i16 [[OR_2]]
 ;
   %or.1 = or disjoint i16 %b, %a

@fhahn fhahn force-pushed the overflow-tracking-check-opcodes branch from 0222f2f to 0ba4571 Compare May 23, 2025 14:00
Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fhahn fhahn merged commit fb923e9 into llvm:main May 28, 2025
11 checks passed
@fhahn fhahn deleted the overflow-tracking-check-opcodes branch May 28, 2025 15:03
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 28, 2025
…Flags (NFC). (#141231)

The logic for tracking flags relies on all instructions having the same
opcode. Add an assert to check, as suggested in
llvm/llvm-project#140406.

PR: llvm/llvm-project#141231
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
llvm#141231)

The logic for tracking flags relies on all instructions having the same
opcode. Add an assert to check, as suggested in
llvm#140406.

PR: llvm#141231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants