hfinkel added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890 + "cannot apply to inline functions, ignoring pragma">, + InGroup<IgnoredPragmas>; ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69272/new/ https://reviews.llvm.org/D69272 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
