[PATCH] D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF

[Warren Ristow via Phabricator via llvm-commits]

wristow marked 5 inline comments as done.
wristow added inline comments.


================
Comment at: test/Transforms/InstCombine/fast-math.ll:61-62
+
+; Verify the fold is not done with only reassoc.
+define double @fold3_reassoc(double %f1) {
+; CHECK-LABEL: @fold3_reassoc(
----------------
spatel wrote:
> x * 2.0 + x --> x * 3.0
> 
> This doesn't require 'nsz'? If x = -0.0, this preserves that the output is -0.0.
> 
> In fact, this exact case doesn't need any fast-ness? (cc @scanon)
> https://reviews.llvm.org/D31164?id=92521#inline-270764
> 
> 
I've updated the comment about 'nsz' not being needed, and I've changed the test so as not to be exercising the special-case of 3*x (and added a comment about the special-case as a TODO, in case anyone ever wants to take advantage of that point).


================
Comment at: test/Transforms/InstCombine/fast-math.ll:99-101
+; Verify the fold is not done with only reassoc.
+define float @fold4_reassoc(float %f1, float %f2) {
+; CHECK-LABEL: @fold4_reassoc(
----------------
spatel wrote:
> (4.0 - x) + (5.0 - y) --> 9.0 - (x + y)
> 
> Again, I don't think this actually requires 'nsz'. Maybe it's better to mark all similar tests with "TODO: We may be able to remove the 'nsz' requirement."
Thanks.  In looking, I think this one //definitely //doesn't require 'nsz', so I've added a TODO comment that's says 'nsz' is unneeded (rather than "may be able to remove...").  Same for many of the others, so I've added similar TODO comments, where appropriate.


================
Comment at: test/Transforms/InstCombine/fast-math.ll:232-235
+define float @fold8_reassoc_nsz(float %f1) {
+; CHECK-LABEL: @fold8_reassoc_nsz(
+; CHECK-NEXT:    [[TMP1:%.*]] = fmul reassoc nsz float [[F1:%.*]], 4.000000e+00
+; CHECK-NEXT:    ret float [[TMP1]]
----------------
spatel wrote:
> See the earlier link - another potential special-case where we don't need any FMF.
I've updated the test to have the opportunity for more folding, so it's not the special-case of x+x+x.
Also, 'nsz' isn't technically required, so I've added a TODO.


================
Comment at: test/Transforms/InstCombine/fast-math.ll:407-414
+; Check again using the minimal subset of FMF (reassoc is sufficient).
+define float @fold14_reassoc(float %x, float %y) {
+; CHECK-LABEL: @fold14_reassoc(
+; CHECK-NEXT:    [[ADD:%.*]] = fsub reassoc float [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret float [[ADD]]
+;
+  %neg = fsub reassoc float -0.0, %x
----------------
spatel wrote:
> -x + y --> y - x
> 
> This doesn't require any FMF.
Good point!  In fact, it //does //fold without any FMF, even without my patch.  I've updated the test to demonstrate the folding without 'fast' (or anything else).


================
Comment at: test/Transforms/InstCombine/fast-math.ll:429-436
+; Check again using the minimal subset of FMF (reassoc is sufficient).
+define float @fold15_reassoc(float %x, float %y) {
+; CHECK-LABEL: @fold15_reassoc(
+; CHECK-NEXT:    [[ADD:%.*]] = fsub reassoc float [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret float [[ADD]]
+;
+  %neg = fsub reassoc float -0.0, %y
----------------
spatel wrote:
> x + (-y) --> x - y
> 
> Again, no FMF needed at all.
As with 'fold14()' above, this does fold without any FMF, even without my patch.  Test updated.


https://reviews.llvm.org/D45453