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: > > 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. > This is an argument in favor of the warning on conflicting attributes. No, because they're not conflicting (as you note, we can still inline), and because the a function might need inline linkage even if the user doesn't care about actual function inlining (this is an unfortunate side effect of the C/C++ specifications providing one keyword for both an optimization hint and a linkage modifier). 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
