[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

Tue Oct 22 07:35:21 PDT 2019

kpn added a comment.

Baking into the front end the fact that the backend implementation is not yet complete doesn't strike me as a good idea.

And the metadata arguments to the constrained intrinsics are designed to allow for correctly marked constrained intrinsics to be eventually treated pretty close to the same as non-constrained math instructions. Once the implementation is further along, of course.

I think the issue with the inliner not being smart enough yet is an issue for llvm to deal with and not front ends like clang. It would be straightforward enough for llvm to mark functions that have the strictfp attribute so they also are marked noinline. As a temporary measure, of course. This is a case where llvm hasn't caught up with well-formed IR, so it would be llvm's job to work around its own incompleteness.

See D43142 <https://reviews.llvm.org/D43142> for code to convert all floating point in a function into constrained intrinsics. Updated versions of this code with support for intrinsics that didn't exist at the time also exist. I don't see why this pass couldn't be reworked a bit to be used by the inliner. And it would only be needed when inlining into a strictfp function a function that wasn't strictfp.

You mentioned that extending the scope of the #pragma may result in a "performance drop, which is unacceptable in many cases". But the only difference between allowing the #pragma only at the top of a function, and allowing it everywhere the standard allows, is that the user knows about the potential loss of performance. The performance loss happens in both cases. Again, I don't think baking into clang the current state of llvm is a good idea.

A warning from clang that strictfp code doesn't perform very well today is probably a good idea, and it would be ripped out easily when the day comes. The warning would only fire when the #pragma is seen, and that code is small, self-contained, and actually already exists in clang now but with different text.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69272