[PATCH] D37019: Add select simplifications

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1410
+            Value *V1 = Builder.CreateFAdd(B1, B2);
+            if (Instruction *I0 = dyn_cast<Instruction>(V0))
+              I0->setFastMathFlags(I.getFastMathFlags());
----------------
craig.topper wrote:
> I don't think this can ever be an Instruction. If you started with two Constants, at worst you'd get back a ConstantExpr here and you need to set the fast math flags on that.
In theory, one could decide not to use the constant folder in the builder so it could happen, couldn't it?


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:721
   // (op (select (a, c, b)), (select (a, d, b))) -> (select (a, (op c, d), 0))
   // (op (select (a, b, c)), (select (a, b, d))) -> (select (a, 0, (op c, d)))
+  Value *A1, *B1, *C1, *A2, *B2, *C2;
----------------
The comment seems weird to me.
Shouldn't it be:
(op (select (a, c, b)), (select (a, d, b))) -> (select (a, (op c, d), (op d, b)))?

Also I don't get why there is two of them :).


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:734
+      if (Value *V = SimplifyBinOp(Opcode, A1, A2, SQ.getWithInstruction(&I)))
+        SI = Builder.CreateSelect(C1, V, Builder.CreateBinOp(Opcode, B1, B2));
+
----------------
When SI != nullptr, shouldn't we use SI  instead of CreateBinOp for B1, B2, here?


================
Comment at: test/Transforms/InstCombine/select_arithmetic.ll:2
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
Could you add a comment on what this test case is checking?


================
Comment at: test/Transforms/InstCombine/select_arithmetic.ll:6
+define float @test1(i1 zeroext) #0 {
+  %2 = select i1 %0, float 5.000000e+00, float 6.000000e+00
+  %3 = select i1 %0, float 1.000000e+00, float 9.000000e+00
----------------
Please use `opt -instnamer` to get rid of the implicit variables.


https://reviews.llvm.org/D37019