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

[Jay Foad via Phabricator via llvm-commits]

foad marked 4 inline comments as done.
foad 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);
+
----------------
spatel wrote:
> 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?
I don't think your suggestions are any better than the status quo; how is anyone supposed to know or remember the difference between `simplifyBinOp` and `simplifyAnyBinOp`? Instead, how about I overload `simplifyBinOp` to take FMF as an optional (last) argument?


================
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
----------------
spatel wrote:
> '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.
True. I could change it to `nsz nnan` if you think that's better. (Incidentally I think `nsz ninf` should also be sufficient in theory, but I won't pursue that in this patch.)


================
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
----------------
spatel wrote:
> Vary the opcodes (fadd, fsub, fdiv) in these tests for better coverage?
`fmul` is fairly unique in having useful simplifications for two different constant operands, 0.0 and 1.0, but I'll see what I can do.


================
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
----------------
spatel wrote:
> Is this test changed by this patch? If not, it shouldn't be here.
Yes. Without this patch, this test does not get simplified at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64713