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

Mon Oct 21 13:55:02 PDT 2019

andrew.w.kaylor added a comment.

Thanks for the patch! I don't have time to review this in detail this week, but I'm very happy to see this functionality.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890
+  "cannot apply to inline functions, ignoring pragma">,
+   InGroup<IgnoredPragmas>;

----------------
rjmccall wrote:
> What's the purpose of this restriction?  Whether `inline` really has much to do with inlining depends a lot on the exact language settings.  (Also, even if this restriction were okay, the diagnostic is quite bad given that there are three separate conditions that can lead to it firing.)
> 
> Also, I thought we were adding instruction-level annotations for this kind of thing to LLVM IR.  Was that not in service of implementing this pragma?
> 
> I'm not categorically opposed to taking patches that only partially implement a feature, but I do want to feel confident that there's a reasonable technical path forward to the full implementation.  In this case, it feels like the function-level attribute is a dead end technically.
I'm guessing this is intended to avoid the optimization problems that would occur (currently) if a function with strictfp were inlined into a function without it. I'm just guessing though, so correct me if I'm wrong.

As I've said elsewhere, I hope this is a temporary problem. It is a real problem though (as is the fact that the inliner isn't currently handling this case correctly).

What would you think of a new command line option that caused us to mark functions with strictfp as noinline? We'd still need an error somewhat like this, but I feel like that would be more likely to accomplish what we want on a broad scale.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69272