[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In https://reviews.llvm.org/D53157#1304895, @cameron.mcinally wrote:

> The problem I was missing is when a FENV_ACCESS=OFF operation, like a FDIV, is hoisted into a FENV_ACCESS=ON region. I see it now, but still think that forcing FENV_ACCESS=OFF operations into constrained intrinsics is a big hammer. If there is a way to add barriers around function calls in a FENV_ACCESS=ON region, that would be better.


It may be a big hammer, but I don't think we'll need to use it terribly often (that is, I don't expect the mixing of FENV_ACCESS regions within a function to be common -- I could be wrong). In any event, we've been clear that choosing to enable FP environment access will mean sacrificing performance in the near term. Once we've had time to teach all the relevant optimizations how to handle the constrained intrinsics the hammer will start to feel much smaller.

In https://reviews.llvm.org/D53157#1304895, @cameron.mcinally wrote:

> Stepping way back, the *best* solution is to have the FPEnv implementation shut down unsafe optimizations on an individual basis. Perhaps we should be tagging functions that contain FENV_ACCESS=ON regions. That way unsafe optimization passes, e.g. hoisting, can query the tag to see if they should skip these functions.


One of the reasons we went with the approach we did is that it provides conservatively correct results by default. Optimizations don't need to be taught to leave the intrinsics alone. They do that as a default behavior when they don't recognize the intrinsic. If we had required optimizations to opt out as needed, we'd have problems chasing down all the places where that needed to happen. So we chose the opposite challenge of using the big hammer for initial implementation and then having to go looking for the places that needed to be taught how to interpret the intrinsics for cases when the optimization can still be performed.

I think at this point we're all on the same page in this regard. I just wanted to make sure we also understand why we're on that page. I still believe it was the correct choice.


https://reviews.llvm.org/D53157