[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 a comment.

Try to organize replies for better references.

**Background**

According to the current design, if floating point operations are represented by constrained intrinsics somewhere in a function, constrained intrinsics must be used in entire function, including inlined function calls (http://lists.llvm.org/pipermail/cfe-dev/2017-August/055325.html). As constrained intrunsics do not participate in optimizations, this may lead to performance drop (discussed in http://lists.llvm.org/pipermail/llvm-dev/2019-August/134641.html). There was an attempt to alleviate this issue using basic block attributes, but this approach was rejected (http://lists.llvm.org/pipermail/llvm-dev/2019-October/135623.html).

In this case allowing `#pragma STDC FENV_ACCESS` at function level seems a good compromise between performance and flexibility. Users get possibility to use non-standard floating point environment and performance impact is limited to the scope  of one function and that scope is controlled by user.

The more general solution, where `#pragma STDC FENV_ACCESS` is allowed in any block inside a function, as the standard requires, would require extending the scope where constrained intrinsics are used. It would result in performance drop, which is unacceptable in many cases. There are ideas to implement the general case using outlining (http://lists.llvm.org/pipermail/llvm-dev/2019-October/135628.html), it could be a promising way to extend functionality of this solution.

**Inlining**

Inlining in an issue for functions with `#pragma STDC FENV_ACCESS`, because if such function is inlined, the host function must use constrained intrinsics, which can result in performance drop. The simplest case is to prohibit inlining of such functions, this way is used in this patch. More elaborated solution would try to merge attributes if possible. For instance, if the host function does not use floating point operations, the function with `#pragma STDC FENV_ACCESS` may be inlined. However it does not look like a job of frontend.

In D69272#1717021 <https://reviews.llvm.org/D69272#1717021>, @kpn wrote:

> Does this work for C++? C++ templates? I only see C tests.


Will add C++ tests soon.

In D69272#1717021 <https://reviews.llvm.org/D69272#1717021>, @kpn wrote:

> Is there a way forward to support having the #pragma at the start of any block inside a function? The effect won't be restricted to that block, true, but the standard does say the #pragma is allowed.


Function outlining may be a general solution.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69272