[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:64
+/// OP: binop with an identity constant
+/// TODO: support for non-commutative and FP opcodes
+static Value *foldSelectInstWithBinaryOp(SelectInst &Sel,
----------------
Add another TODO for the 'ne' predicate.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:66
+static Value *foldSelectInstWithBinaryOp(SelectInst &Sel,
+                                         InstCombiner::BuilderTy &Builder) {
+  Value *Cond = Sel.getCondition();
----------------
We don't need the builder here. In fact, we don't even need to create a new instruction. Just replace the true value of the select with 'Z' and return &Sel to indicate that it was morphed. 

This preserves metadata that this patch would unknowingly drop (please add a test for that). There are examples of tests with select transforms that have metadata in:
test/Transforms/InstCombine/select_meta.ll


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:71
+
+  Value *X, *Z, *C;
+  CmpInst::Predicate Pred;
----------------
'C' should be declared and matched as a Constant type.


================
Comment at: test/Transforms/InstCombine/select-binop-icmp.ll:174-175
+;
+  %A = icmp eq <2 x i8>  %x, <i8 0, i8 undef>
+  %B = xor <2 x i8>  %x, %z
+  %C = select <2 x i1>  %A, <2 x i8>  %B, <2 x i8>  %y
----------------
This case should fold, right? Please add a TODO comment here and in the source if that's correct.


https://reviews.llvm.org/D49954