Skip to content

[InstCombine] Consolidate another fold into select value equivalence #117746

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 1 commit into from
Dec 2, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Nov 26, 2024

We had a separate fold that handled just the trivial case where we're replacing exactly the argument of the select. Handle this in select value equivalence by relaxing the infinite loop protection to allow a replacement of a non-constant with a constant.

This also fixes #113301, as the separate fold did not handle undef values correctly.

We had a separate fold that handled just the trivial case where
we're replacing exactly the argument of the select. Handle this
in select value equivalence by relaxing the infinite loop protection
to allow a replacement of a non-constant with a constant.

This also fixes llvm#113301,
as the separate fold did not handle undef values correctly.
@llvmbot
Copy link
Member

llvmbot commented Nov 26, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

We had a separate fold that handled just the trivial case where we're replacing exactly the argument of the select. Handle this in select value equivalence by relaxing the infinite loop protection to allow a replacement of a non-constant with a constant.

This also fixes #113301, as the separate fold did not handle undef values correctly.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+1-12)
  • (modified) llvm/test/Transforms/InstCombine/select-value-equivalence.ll (+1-2)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index e5525133e5dbb5..245fecb775e8c0 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1348,7 +1348,7 @@ Instruction *InstCombinerImpl::foldSelectValueEquivalence(SelectInst &Sel,
     // If we will be able to evaluate f(Y) to a constant, we can allow undef,
     // otherwise Y cannot be undef as we might pick different values for undef
     // in the cmp and in f(Y).
-    if (TrueVal == OldOp)
+    if (TrueVal == OldOp && (isa<Constant>(OldOp) || !isa<Constant>(NewOp)))
       return nullptr;
 
     if (Value *V = simplifyWithOpReplaced(TrueVal, OldOp, NewOp, SQ,
@@ -1925,17 +1925,6 @@ Instruction *InstCombinerImpl::foldSelectInstWithICmp(SelectInst &SI,
   ICmpInst::Predicate Pred = ICI->getPredicate();
   Value *CmpLHS = ICI->getOperand(0);
   Value *CmpRHS = ICI->getOperand(1);
-  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
-      replaceOperand(SI, 1, CmpRHS);
-      Changed = true;
-    } else if (CmpLHS == FalseVal && Pred == ICmpInst::ICMP_NE) {
-      // Transform (X != C) ? Y : X -> (X != C) ? Y : C
-      replaceOperand(SI, 2, CmpRHS);
-      Changed = true;
-    }
-  }
 
   if (Instruction *NewSel = foldSelectICmpEq(SI, ICI, *this))
     return NewSel;
diff --git a/llvm/test/Transforms/InstCombine/select-value-equivalence.ll b/llvm/test/Transforms/InstCombine/select-value-equivalence.ll
index d55766ddf40405..812b4d8f6be793 100644
--- a/llvm/test/Transforms/InstCombine/select-value-equivalence.ll
+++ b/llvm/test/Transforms/InstCombine/select-value-equivalence.ll
@@ -322,12 +322,11 @@ define <2 x i8> @select_vec_op_const_no_undef(<2 x i8> %x) {
   ret <2 x i8> %xr
 }
 
-; FIXME: This is a miscompile.
 define <2 x i8> @select_vec_op_const_undef(<2 x i8> %x) {
 ; CHECK-LABEL: define <2 x i8> @select_vec_op_const_undef(
 ; CHECK-SAME: <2 x i8> [[X:%.*]]) {
 ; CHECK-NEXT:    [[XZ:%.*]] = icmp eq <2 x i8> [[X]], <i8 1, i8 undef>
-; CHECK-NEXT:    [[XR:%.*]] = select <2 x i1> [[XZ]], <2 x i8> <i8 1, i8 undef>, <2 x i8> <i8 4, i8 3>
+; CHECK-NEXT:    [[XR:%.*]] = select <2 x i1> [[XZ]], <2 x i8> [[X]], <2 x i8> <i8 4, i8 3>
 ; CHECK-NEXT:    ret <2 x i8> [[XR]]
 ;
   %xz = icmp eq <2 x i8> %x, <i8 1, i8 undef>

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.

@nikic nikic merged commit 7bbc049 into llvm:main Dec 2, 2024
11 checks passed
@nikic nikic deleted the instcombine-select-replacement branch December 2, 2024 08:45
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.

Wrong canonicalization of add and bitwise logic operation
3 participants