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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
