[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);
+
----------------
foad wrote:
> 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?
Ok, that's less misleading than what we have now. You can make that change without review, then rebase this patch.


================
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
----------------
foad wrote:
> 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.
Ok, then please commit this test with baseline CHECK lines as another preliminary NFC step ahead of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64713