Skip to content
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

[InstCombine] Fix worklist management in select fold #77738

Merged
merged 1 commit into from Jan 11, 2024

Conversation

ParkHanbum
Copy link
Contributor

@ParkHanbum ParkHanbum commented Jan 11, 2024

InstCombine uses Worklist to manage change history. setOperand, which was previously used to change the Select Instruction, does not, so it is run twice, which causes an LLVM ERROR.

This problem is resolved by changing setOperand to replaceOperand as the change history will be registered in the Worklist.

Fixes #77553.

`InstCombine` uses `Worklist` to manage change history. `setOperand`,
which was previously used to change the `Select` Instruction, does not,
so it is `run` twice, which causes an `LLVM ERROR`.

This problem is resolved by changing `setOperand` to `replaceOperand`
as the change history will be registered in the Worklist.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 11, 2024

@llvm/pr-subscribers-llvm-transforms

Author: hanbeom (ParkHanbum)

Changes

InstCombine uses Worklist to manage change history. setOperand, which was previously used to change the Select Instruction, does not, so it is run twice, which causes an LLVM ERROR.

This problem is resolved by changing setOperand to replaceOperand as the change history will be registered in the Worklist.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+2-2)
  • (modified) llvm/test/Transforms/InstCombine/select.ll (+14)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 2dda46986f0fd0..1aa47b2ccfd82b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1703,11 +1703,11 @@ Instruction *InstCombinerImpl::foldSelectInstWithICmp(SelectInst &SI,
   if (CmpRHS != CmpLHS && isa<Constant>(CmpRHS) && !isa<Constant>(CmpLHS)) {
     if (CmpLHS == TrueVal && Pred == ICmpInst::ICMP_EQ) {
       // Transform (X == C) ? X : Y -> (X == C) ? C : Y
-      SI.setOperand(1, CmpRHS);
+      replaceOperand(SI, 1, CmpRHS);
       Changed = true;
     } else if (CmpLHS == FalseVal && Pred == ICmpInst::ICMP_NE) {
       // Transform (X != C) ? Y : X -> (X != C) ? Y : C
-      SI.setOperand(2, CmpRHS);
+      replaceOperand(SI, 2, CmpRHS);
       Changed = true;
     }
   }
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index 7583a75385a769..5687371b4e1dd2 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -3646,3 +3646,17 @@ loop:
 exit:
   ret i32 %rem
 }
+
+; (X == C) ? X : Y -> (X == C) ? C : Y
+; Fixed #77553
+define i32 @src_select_xxory_eq0_xorxy_y(i32 %x, i32 %y) {
+; CHECK-LABEL: @src_select_xxory_eq0_xorxy_y(
+; CHECK-NEXT:    [[XOR0:%.*]] = icmp eq i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[XOR0]], i32 0, i32 [[Y]]
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+  %xor = xor i32 %x, %y
+  %xor0 = icmp eq i32 %xor, 0
+  %cond = select i1 %xor0, i32 %xor, i32 %y
+  ret i32 %cond
+}

@dtcxzyw dtcxzyw linked an issue Jan 11, 2024 that may be closed by this pull request
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. I tested it on my local machine.

@nikic nikic changed the title [InstCombine] Fix run twice at once (#77553) [InstCombine] Fix worklist management in select fold Jan 11, 2024
@nikic nikic merged commit 66eedd1 into llvm:main Jan 11, 2024
4 of 5 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
`InstCombine` uses `Worklist` to manage change history. `setOperand`,
which was previously used to change the `Select` Instruction, does not,
so it is `run` twice, which causes an `LLVM ERROR`.

This problem is resolved by changing `setOperand` to `replaceOperand` as
the change history will be registered in the Worklist.

Fixes llvm#77553.
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.

LLVM ERROR while InstCombine
4 participants