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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: scanon.
spatel added a comment.

In https://reviews.llvm.org/D45453#1066811, @wristow wrote:

> I've updated the code-change in "InstCombineAddSub.cpp" to require both 'reassoc' and 'nsz' to do the transformations.  (I haven't changed the name from FAddCombine to FAddSubCombine or ReassociateFAddFSub.  I could do that if we move forward with this.)
>
> >> There could be some kind of symbiotic behavior here, but this is really saying that we don't have a well-defined division of labor between these passes, right?
> > 
> > Yes, a lack of a well-defined division of labor between these passes is exactly my concern. I'll look into adding or modifying tests. Depending on possible interaction, that may not be fruitful. Ultimately, that should be addressed.
>
> In looking this over, I was pleasantly surprised that I didn't run into the troublesome interaction I was concerned about.  So I've added new tests to "InstCombine/fast-math.ll" to check for the transformations happening when 'reassoc' + 'nsz' are on (with only '-instcombine' used).  I've also added tests that verify that 'reassoc' alone doesn't enable the transformation in the appropriate places.  Lastly, I updated the tests I had previously modified to use both 'reassoc' and 'nsz' where appropriate when testing for the minimal set of needed flags to transform things.


Thanks - more instcombine tests in this area are needed IMO.

I think the code change is fine in that it removes the unrelated FMF from these transforms. And it may be that after this change, we're 'good enough'. Ie, it may be that nobody cares about splitting the FMF hairs further than this.

But I have pointed out some test comments that I don't think are correct. I didn't look at all tests, but I suggest softening the language in general and adding 'TODO', so if someone does decide to improve more, the tests acknowledge that what we have currently isn't complete.



================
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(
----------------
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




================
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(
----------------
(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."


================
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]]
----------------
See the earlier link - another potential special-case where we don't need any FMF.


================
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
----------------
-x + y --> y - x

This doesn't require any FMF.


================
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
----------------
x + (-y) --> x - y

Again, no FMF needed at all.


https://reviews.llvm.org/D45453