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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> I'm unclear on exactly the semantics of any of these is supposed to be. "no-builtins" isn't in the LangRef, and the wording on nobuiltin is oddly call site specific. With this confusion, I think the implementation is just buggy. What is the difference between nobuiltin and "no-builtins" supposed to be? The nobuiltin LangRef merely states it may be placed on declarations and definitions, but doesn't state what that means in that case.

Roughly speaking, nobuiltin exists to suppress libcall recognition on a specific call, and "no-builtins" suppresses libcall recognition in function definitions.  Sort of similar, but not exactly the same.  (I think the primary case where we need "nobuiltin" C++ operator new, where a new expression and an explicit call to operator new have different optimization rules.)

----

> I looked into this and it's just a straight SelectionDAG bug. DAG.getMemcpy tries expanding inline depending on target size thresholds and custom instructions, but ultimately unconditionally emits an illegal libcall

memcpy in particular has its own set of considerations.  Historically, we required the user to provide a symbol named "memcpy", no matter what flags they passed.  This has turned out to be unsatisfactory in certain scenarios, so we've worked around it.  The primary form of workaround is that we don't form memcpys; if the input IR doesn't contain any reference to llvm.memcpy, we don't introduce such a reference.  This makes optimizations slightly worse, but basically avoids the weird issues without significantly pessimizing code in most cases.  (memcpy formation is pretty niche in most cases.)

A codepath technically exists added to expand memcpy, but it generates extremely bad code, to the point where it's not actually usable unless you don't care at all about performance.  So practically speaking, nothing has changed here.

-----

> but no equivalent mechanism is used for "RuntimeLibcalls"

Historically, we assumed that all the functions in RuntimeLibcalls were available even in no-builtins mode, because we don't have alternative implementations.  This has always had various holes, but it was a good enough approximation to allow building system libraries like libc.

----

> Was there a reason that we could not simply merge the nobuiltin attribute into the caller?

The primary issue with LTO'ing parts of libc into a program is that we're basically crossing a line: once we start inlining calls into the program, "libc" is no longer an abstract interface, so we have to turn off libcall recognition for the whole module.  I don't think merging the nobuiltin markings when you inline a function is sufficient.

----

This discussion is going in so many different directions it's hard for me to address everything...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74162