[PATCH] D37019: Add select simplifications

[David Majnemer via Phabricator via llvm-commits]

majnemer requested changes to this revision.
majnemer added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:735
+    if (A1 == A2) {
+      if (CarryFastMathFlags)
+        Builder.setFastMathFlags(I.getFastMathFlags());
----------------
I think you always pass `true` for `CarryFastMathFlags`. Is the goal to avoid invalid access to `getFastMathFlags `? If so, I'd instead do something like `if (isa<FPMathOperator>(&I))`.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:736
+      if (CarryFastMathFlags)
+        Builder.setFastMathFlags(I.getFastMathFlags());
+
----------------
Forgive me if I am wrong but wouldn't this mutate the state of the builder beyond your intended use? I think you need a `IRBuilder<>::FastMathFlagGuard`.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:740
+      Value *V1 = SimplifyBinOp(Opcode, C, E, SQ.getWithInstruction(&I));
+      if (V1 != nullptr)
+        SI = Builder.CreateSelect(A1, Builder.CreateBinOp(Opcode, B, D), V1);
----------------
I think the common custom in LLVM is to simply say `if (V1)`.


================
Comment at: test/Transforms/InstCombine/select_arithmetic.ll:41-47
+attributes #0 = { noinline nounwind ssp uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="penryn" "target-features"="+cx16,+fxsr,+mmx,+sse,+sse2,+sse3,+sse4.1,+ssse3,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"Apple LLVM version 9.0.0 (clang-900.0.32)"}
----------------
I don't think these add to the test, I'd remove them.


https://reviews.llvm.org/D37019