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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked an inline comment as done.
sepavloff added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890
+  "cannot apply to inline functions, ignoring pragma">,
+   InGroup<IgnoredPragmas>;
 
----------------
hfinkel wrote:
> sepavloff wrote:
> > hfinkel wrote:
> > > 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).
> > As Andrew already said, `noinline` attribute is a mean to limit negative performance impact. Of course, to inline or not to inline - such decision is made by backend. However it a user requested a function to be inline, a warning looks useful.
> > 
> > When constrained intrinsics get full support in optimizations, this restriction will become unnecessary.
> > 
> > Outlining is one of the ways that converts this solution into full pragma implementation. Another is implementation of constrained intrinsics support in optimization transformations.
> > 
> > As for a new command line option that caused us to mark functions with strictfp as noinline, it loos a good idea, but we must adapt inliner first, so that it can convert ordinary floating operations to constrained intrinsics during inlining.
> > 
> > In the case of `#pragma STDC FENV_ACCESS` we cannot in general say if attributes are compatible so a function can be inlined into another. The pragma only says that user modified floating point environment. One function may set only rounding mode and another use different exception handling, in this case we cannot do inlining. More fine grained pragmas, like that proposed in https://reviews.llvm.org/D65997 could enable more flexible inlining.
> > Of course, to inline or not to inline - such decision is made by backend. However it a user requested a function to be inline, a warning looks useful.
> 
> We need to be careful. inline is not just an optimizaiton hint. It also affects function linkage. Also, when inlining into functions with similar constraints, there's no problem with the inlining.
> 
> > One function may set only rounding mode and another use different exception handling, in this case we cannot do inlining.
> 
> Can you please explain this? It does not seem like that should block inlining when both the caller and callee are marked as fenv_access-enabled.
> We need to be careful. inline is not just an optimizaiton hint. It also affects function linkage. Also, when inlining into functions with similar constraints, there's no problem with the inlining.
This is an argument in favor of the warning on conflicting attributes.

>> One function may set only rounding mode and another use different exception handling, in this case we cannot do inlining.
>Can you please explain this? It does not seem like that should block inlining when both the caller and callee are marked as fenv_access-enabled. 

I was wrong. If a function correctly restores FP state, it can be inlined into another fenv_access-enabled function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69272