[PATCH] D129523: [Reassociate] Enable FP reassociation via 'reassoc' and 'nsz'

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

Thanks for the quick feedback!

> Using `I->isAssociative()` directly seems like the right change rather than adding a wrapper (agree with inline comment from @nikic). Any uses of `isFast()` in IR are bogus at this point (that API should be deprecated) - there is no transform that requires all of the individual FMF.

I'll update the check to look for `hasAllowReassoc() && hasNoSignedZeros()` rather than using `isFast()` at all, and rather than invoking `isAssociative()` (described in more detail in my inline comment).

> If we can fix the regressions as an independent patch, that would be ideal.

I'll split this into two independent patches.



================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:151
+  assert(I && I->getType()->isFPOrFPVectorTy() && "Should only check FP ops");
+  return I->isFast() || I->isAssociative();
+}
----------------
nikic wrote:
> Just `I->isAssociative()` is sufficient, `isFast()` implies `isAssociative()`.
Actually that is what I tried in my first attempt, but it didn't work. :)
The problem is that there are operators that are not associative (and so `isAssociative()` returns `false`), but some of those operators can be transformed into something associative, and the Reassociation Pass then nicely handles them.  For example, subtraction isn't associative, but if we have:
```
(0.0-X)*Y + Z
```
then this can (arithmetically) be transformed into:
```
Z - X*Y
```
But that transformation was suppressed when `isAssociative()` alone was used, because it returned `false`.  So for example, code like the following:
```
%A = fsub fast float 0.0, %X
%B = fmul fast float %A, %Y
%C = fadd fast float %B, %Z
```
no longer was optimized when I only used `isAssociative()`.

But as I write this, I realize that by including `isFast()`, that's not the right solution.  I //should// have done is check for `reassoc` and `nsz`:
`hasAllowReassoc() && hasNoSignedZeros()`
rather than `isFast()`.  I'll change that.  That will optimize more cases.


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

https://reviews.llvm.org/D129523