[PATCH] D50714: [InstCombine] Fold Select with binary op - FP opcodes

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think we're almost done! Just a few nits inline.



================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:95
+    if (!BO->hasNoSignedZeros() && !CannotBeNegativeZero(Y, &TLI))
+    return nullptr;
+
----------------
indent


================
Comment at: test/Transforms/InstCombine/select-binop-cmp.ll:193
+
+define float @select_fadd_fcmp_5(float %x, float %y, float %v) {
+; CHECK-LABEL: @select_fadd_fcmp_5(
----------------
This is identical to select_fadd_fcmp_2? Did you mean to check OEQ instead?


================
Comment at: test/Transforms/InstCombine/select-binop-cmp.ll:583-584
 
+define float @select_fadd_fcmp_bad_5(float %x, float %y, float %z) {
+; CHECK-LABEL: @select_fadd_fcmp_bad_5(
+; CHECK-NEXT:    [[A:%.*]] = fcmp one float [[X:%.*]], -0.000000e+00
----------------
The 'bad' tests provide good coverage for all of the ways this transform should be limited, but FP is complicated, and it's very difficult to know exactly which condition in the test causes us to bypass the transform - and that's with this patch fresh in memory...we probably won't remember any of this in a month. :) 

Please add a short comment above each test to explain why we can't do the fold for that test.


https://reviews.llvm.org/D50714