[PATCH] Module: Use existing library functions
mail at justinbogner.com
Mon Mar 3 13:15:52 PST 2014
"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
> During LTO, user-supplied definitions of C library functions often
> exist. -instcombine uses Module::getOrInsertFunction() to get a handle
> on library functions (e.g., @puts, when optimizing @printf).
> Previously, Module::getOrInsertFunction() would rename any matching
> functions with local linkage, and create a new declaration. In LTO,
> this is the opposite of desired behaviour, as it skips by the
> user-supplied version of the library function and creates a new
> undefined reference which the linker often cannot resolve.
Why do the user-supplied versions have local linkage? That seems a bit
odd to me, but I'm not all that familiar with the details of LTO.
> Removing that logic did not cause any tests to fail, and I couldn't
> think of why it's *good* behaviour. Maybe no one was using it?
As far as I can tell there are three use cases for getOrInsertFunction:
1. Lazily creating a function that we want to manipulate - the first
call of getOrInsertFunction inserts, and subsequent calls get. The
"insert" logic creates a function with ExternalLinkage, so the
removed code path shouldn't occur for this usage. On the other hand,
if someone is attempting to do this and a function with local linkage
already exists with that name, the result will be completely broken,
since we'll get a large number of copies of the function we intended
2. Accessing a function with a well known name, which expect will
already exist. Creating a function here doesn't make much sense, so
the renaming behaviour also seems broken, but it's surprising to me
that such a function would have local linkage, as above, so I don't
see why we hit this.
3. Creating a unique function. In this came I guess the name is only
there to help with debugging, so the behaviour we currently have kind
of makes sense. This is a bit silly though, since the "get" part of
getOrInsertFunction would never be wanted. I'd expect callers that
need this (if there are any) to just do Function::Create
So basically, I can't think of how this would be good behaviour either,
and there are at least two ways in which it's bad.
> I added two testcases: one specifically for the -instcombine behaviour
> and one for the LTO flow that exposed it.
More information about the llvm-commits