[LICM] Drop poison-generating flags when reassociating an icmp#200344
Merged
Conversation
hoistAdd/hoistSub turn `LV + C1 <pred> C2` into `LV <pred> C2 - C1`, changing the icmp's LHS. A samesign flag asserted about the old operands need not hold for the new LHS, so keeping it can turn a defined comparison into poison. Drop the icmp's poison-generating flags after the rewrite, as hoistMulAddAssociation already does. This bug was found by a large run of Opus 4.7 looking for bugs in LLVM. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
59b3840 to
dd80530
Compare
|
@llvm/pr-subscribers-llvm-transforms Author: Justin Lebar (jlebar) Changes
This bug was found by a large run of Opus 4.7 looking for bugs in LLVM. Full diff: https://github.com/llvm/llvm-project/pull/200344.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index be92339052ec5..e9e3554959a67 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -2602,6 +2602,9 @@ static bool hoistAdd(ICmpInst::Predicate Pred, Value *VariantLHS,
ICmp.setPredicate(Pred);
ICmp.setOperand(0, VariantOp);
ICmp.setOperand(1, NewCmpOp);
+ // The new LHS is a different value, so a samesign (or any other
+ // poison-generating) flag asserted about the old operands may no longer hold.
+ ICmp.dropPoisonGeneratingFlags();
Instruction &DeadI = cast<Instruction>(*VariantLHS);
salvageDebugInfo(DeadI);
@@ -2683,6 +2686,9 @@ static bool hoistSub(ICmpInst::Predicate Pred, Value *VariantLHS,
ICmp.setPredicate(Pred);
ICmp.setOperand(0, VariantOp);
ICmp.setOperand(1, NewCmpOp);
+ // The new LHS is a different value, so a samesign (or any other
+ // poison-generating) flag asserted about the old operands may no longer hold.
+ ICmp.dropPoisonGeneratingFlags();
Instruction &DeadI = cast<Instruction>(*VariantLHS);
salvageDebugInfo(DeadI);
diff --git a/llvm/test/Transforms/LICM/hoist-add-sub.ll b/llvm/test/Transforms/LICM/hoist-add-sub.ll
index c9b8977b73306..aed67e4faed48 100644
--- a/llvm/test/Transforms/LICM/hoist-add-sub.ll
+++ b/llvm/test/Transforms/LICM/hoist-add-sub.ll
@@ -1007,11 +1007,11 @@ define void @or_disjoint_signed(ptr %x_p) {
; CHECK: loop:
; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[BACKEDGE:%.*]] ]
; CHECK-NEXT: [[X_CHECK:%.*]] = icmp slt i32 [[IV]], [[INVARIANT_OP]]
-; CHECK-NEXT: br i1 [[X_CHECK]], label [[OUT_OF_BOUNDS:%.*]], label [[BACKEDGE]]
+; CHECK-NEXT: br i1 [[X_CHECK]], label [[EXIT:%.*]], label [[BACKEDGE]]
; CHECK: backedge:
; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 4
; CHECK-NEXT: [[LOOP_COND:%.*]] = icmp slt i32 [[IV_NEXT]], 1000
-; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT:%.*]]
+; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT]]
; CHECK: exit:
; CHECK-NEXT: ret void
;
@@ -1045,11 +1045,11 @@ define void @or_disjoint_unsigned(ptr %x_p) {
; CHECK: loop:
; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[BACKEDGE:%.*]] ]
; CHECK-NEXT: [[X_CHECK:%.*]] = icmp ult i32 [[IV]], [[INVARIANT_OP]]
-; CHECK-NEXT: br i1 [[X_CHECK]], label [[OUT_OF_BOUNDS:%.*]], label [[BACKEDGE]]
+; CHECK-NEXT: br i1 [[X_CHECK]], label [[EXIT:%.*]], label [[BACKEDGE]]
; CHECK: backedge:
; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 4
; CHECK-NEXT: [[LOOP_COND:%.*]] = icmp ult i32 [[IV_NEXT]], 1000
-; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT:%.*]]
+; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT]]
; CHECK: exit:
; CHECK-NEXT: ret void
;
@@ -1083,11 +1083,11 @@ define void @or_no_disjoint_neg(ptr %x_p) {
; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[BACKEDGE:%.*]] ]
; CHECK-NEXT: [[ARITH:%.*]] = or i32 [[IV]], [[X]]
; CHECK-NEXT: [[X_CHECK:%.*]] = icmp slt i32 [[ARITH]], 100
-; CHECK-NEXT: br i1 [[X_CHECK]], label [[OUT_OF_BOUNDS:%.*]], label [[BACKEDGE]]
+; CHECK-NEXT: br i1 [[X_CHECK]], label [[EXIT:%.*]], label [[BACKEDGE]]
; CHECK: backedge:
; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 4
; CHECK-NEXT: [[LOOP_COND:%.*]] = icmp slt i32 [[IV_NEXT]], 1000
-; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT:%.*]]
+; CHECK-NEXT: br i1 [[LOOP_COND]], label [[LOOP]], label [[EXIT]]
; CHECK: exit:
; CHECK-NEXT: ret void
;
@@ -1114,3 +1114,36 @@ exit:
!1 = !{i32 0, i32 2147483640}
!2 = !{i32 256, i32 32768}
!3 = !{i32 0, i32 2}
+
+; The reassociation changes the icmp's LHS, so a samesign flag asserted about
+; the original operands must be dropped (otherwise it can introduce poison).
+define void @test_samesign_dropped(ptr %p, i32 %x) {
+; CHECK-LABEL: define void @test_samesign_dropped
+; CHECK-SAME: (ptr [[P:%.*]], i32 [[X:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: [[IV:%.*]] = phi i32 [ [[X]], [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP]] ]
+; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[IV]], 95
+; CHECK-NEXT: [[SEL:%.*]] = zext i1 [[CMP]] to i32
+; CHECK-NEXT: store volatile i32 [[SEL]], ptr [[P]], align 4
+; CHECK-NEXT: [[IV_NEXT]] = add nsw i32 [[IV]], 1
+; CHECK-NEXT: [[DONE:%.*]] = icmp eq i32 [[IV_NEXT]], 0
+; CHECK-NEXT: br i1 [[DONE]], label [[EXIT:%.*]], label [[LOOP]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %loop
+loop:
+ %iv = phi i32 [ %x, %entry ], [ %iv.next, %loop ]
+ %sum = add nsw i32 %iv, 5
+ %cmp = icmp samesign slt i32 %sum, 100
+ %sel = zext i1 %cmp to i32
+ store volatile i32 %sel, ptr %p
+ %iv.next = add nsw i32 %iv, 1
+ %done = icmp eq i32 %iv.next, 0
+ br i1 %done, label %exit, label %loop
+exit:
+ ret void
+}
|
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
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.
hoistAdd/hoistSubturnLV + C1 <pred> C2intoLV <pred> C2 - C1, changing the icmp's LHS. Asamesignflag asserted about the old operands need not hold for the new LHS, so keeping it can turn a defined comparison into poison (e.g. for%iv = -3,samesign slt(2, 100)is defined-true but the reassociatedsamesign slt(-3, 95)has opposite-sign operands → poison). Drop the icmp's poison-generating flags after the rewrite, ashoistMulAddAssociationalready does.This bug was found by a large run of Opus 4.7 looking for bugs in LLVM.