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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890
+  "cannot apply to inline functions, ignoring pragma">,
+   InGroup<IgnoredPragmas>;
 
----------------
andrew.w.kaylor wrote:
> 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.
We would not want to prevent all inlining, just inlining where the attributes don't match. We should fix his first. I think we just need to add a CompatRule to include/llvm/IR/Attributes.td (or something like that).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69272