[PATCH] D64713: [InstCombine] X *fast (C ? 1.0 : 0.0) -> C ? X : 0.0

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:778-779
+    Cond = A;
+    True = SimplifyFPBinOp(Opcode, B, E, FMF, Q);
+    False = SimplifyFPBinOp(Opcode, C, F, FMF, Q);
+
----------------
I know this is an existing function name, but it doesn't match my expectation based on the name. Change it to simplifyAnyBinOp() or simplifyIntOrFPBinOp() as a preliminary step?


================
Comment at: llvm/test/Transforms/InstCombine/fmul.ll:1005
+  %sel = select i1 %c, float 1.0, float 0.0
+  %mul = fmul fast float %sel, %x
+  ret float %mul
----------------
'fast' is over-specifying the constraints here (and probably all of these tests). 
fmul by 1.0 doesn't need any flags; fmul by 0.0 only needs nsz and nnan.


================
Comment at: llvm/test/Transforms/InstCombine/fmul.ll:1016
+  %sel = select i1 %c, <2 x float> <float 1.0, float 1.0>, <2 x float> zeroinitializer
+  %mul = fmul fast <2 x float> %sel, %x
+  ret <2 x float> %mul
----------------
Vary the opcodes (fadd, fsub, fdiv) in these tests for better coverage?


================
Comment at: llvm/test/Transforms/InstCombine/mul.ll:522-530
+define i32 @mul_div_select(i32 %x, i32 %y, i1 %c) {
+; CHECK-LABEL: @mul_div_select(
+; CHECK-NEXT:    [[MUL:%.*]] = select i1 [[C:%.*]], i32 [[X:%.*]], i32 [[Y:%.*]]
+; CHECK-NEXT:    ret i32 [[MUL]]
+;
+  %div = udiv exact i32 %x, %y
+  %sel = select i1 %c, i32 %div, i32 1
----------------
Is this test changed by this patch? If not, it shouldn't be here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64713/new/

https://reviews.llvm.org/D64713