Skip to content

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.
@nikic nikic requested review from dtcxzyw and goldsteinn November 26, 2024 16:52
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Nov 26, 2024
@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

Labels

llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong canonicalization of add and bitwise logic operation

3 participants