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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:151
+  assert(I && I->getType()->isFPOrFPVectorTy() && "Should only check FP ops");
+  return I->isFast() || I->isAssociative();
+}
----------------
wristow wrote:
> 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.
I think there was some confusion about what `isAssociative` is. It's based on opcode not the fast math flags. It's a property like isCommutative.


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

https://reviews.llvm.org/D129523