[PATCH] D39304: [IR] redefine 'reassoc' fast-math-flag and add 'trans' fast-math-flag

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D39304#1002599, @qcolombet wrote:

> Hi Sanjay,
>
> > Did this manifest as a performance problem only or was there a visible functional difference too?
>
> I guess it depends how you qualify visible functional difference :).
>  With the previous layout of the bits, reassociation was performed, but with the new layout, it is not.
>  The reason behind that reassociation checks isFast, which with the new layout is true only if all the 7 bits are set to true and that doesn't happen when the upgrade path is used (we get only 5 of them).
>
> So this is a performance problem that is exposed by a functional difference in the compiler (an optimization was triggered and now it is not).


Right - we expected that could happen with existing IR compiled with -ffast-math. I don't think we could go **wrong** in that scenario given that 'fast' gives us license to do all kinds of transforms, but doesn't require it - although people have different expectations once they get accustomed to the optimizations. :)

> Given this new format was not released yet, do you see a way we could make the autoupgrade path to preserve the isFast semantic from the old format to the new format?
>  I haven't audited the code, but potential we are going to regress all the code that relied on isFast and given this is not publicly available yet (unless I am mistaken), I was wondering if there is a way to fix that.

Michael has this patch already, right? I think we have to create a new version of the IR since the bits changed meaning (we can't just flip 'on' new bits).

I have no objection to that, but that use case wasn't important to me, so that's why I didn't bother in this patch. AFAIK, this is new for v6.0, so yes, it's still possible to do this before that window closes.


Repository:
  rL LLVM

https://reviews.llvm.org/D39304