[PATCH] D74162: [Inliner] Inlining should honor nobuiltin attributes

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:267
+  /// attributes, which is conservatively correct.
+  bool areInlineCompatible(const TargetLibraryInfo &CalleeTLI) const {
+    BitVector B = OverrideAsUnavailable;
----------------
davidxl wrote:
> tejohnson wrote:
> > davidxl wrote:
> > > This may be bad for performance -- as the inline instance will be optimized differently.
> > Note that it won't be worse than head, which doesn't restrict the inlines based on nobuiltin attributes at all. We could also just disallow inlining completely between callers/callees with different nobuiltin attributes. But I was concerned that this would degrade performance too much by disallowing inlining in too many cases.
> Perhaps add an additional parameter to the interface to allow superset behavior. Then in the inlineCost.cpp, add an internal option to specify whether strict  attribute matching is required -- the default can be the current behavior -- allow inlining into superset.
> Note that it won't be worse than head, which doesn't restrict the inlines based on nobuiltin attributes at all. We could also just disallow inlining completely between callers/callees with different nobuiltin attributes. But I was concerned that this would degrade performance too much by disallowing inlining in too many cases.

I agree disallowing inlining completely when `nobuiltin` differ would prevent inlining of basic memory functions entirely (memset, memcpy, etc..)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74162