[PATCH] D49954: [InstCombine] Fold Select with binary op

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:66
+static Instruction *
+foldSelectInstWithBinaryOp(SelectInst &Sel, InstCombiner::BuilderTy &Builder) {
+  Value *Cond = Sel.getCondition();
----------------
foldSelectBinOpIdentity() would be a more descriptive name.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:74-75
+  CmpInst::Predicate Pred;
+  if (match(Cond, m_ICmp(Pred, m_Value(X), m_Constant(C)))) {
+    if (Pred == ICmpInst::ICMP_EQ) {
+      if (match(TrueVal, m_c_BinOp(m_Specific(X), m_Value(Z))) &&
----------------
I'd like to avoid the code duplication if it doesn't make it too messy. How about exiting if the compare doesn't match what we need:

```
 if (!match(Cond, m_ICmp(Pred, m_Value(X), m_Constant(C))) ||
     !ICmpInst::isEquality(Pred))
  return nullptr;
```

Then choose the select operand based on whether:
bool IsEq = Pred == ICmpInst::ICMP_EQ;


================
Comment at: test/Transforms/InstCombine/select-binop-icmp.ll:102-106
+; CHECK-NEXT:    [[A:%.*]] = icmp eq <2 x i8> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    [[C:%.*]] = select <2 x i1> [[A]], <2 x i8> [[Z:%.*]], <2 x i8> [[Y:%.*]]
 ; CHECK-NEXT:    ret <2 x i8> [[C]]
 ;
+  %A = icmp ne <2 x i8>  %x, <i8 0, i8 0>
----------------
Is the icmp predicate getting canonicalized to 'eq' before we reach this transform?

Add an extra use of the icmp with something like:
call @use(i1 %A)
...to make sure we have coverage for the 'ne' side of the transform.
(and commit the test diffs before this patch, no pre-commit review needed on more tests)


https://reviews.llvm.org/D49954