[PATCH] Add an -mattr option to the gold plugin to support subtarget features in LTO

[Tom Roeder]

On Thu, Mar 27, 2014 at 2:54 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Nick Kledzik wrote:
>>
>>
>> On Mar 25, 2014, at 12:06 AM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>
>>> Nick Kledzik wrote:
>>>>
>>>>
>>>> On Mar 24, 2014, at 4:27 PM, Tom Roeder<tmroeder at google.com>   wrote:
>>>>>
>>>>> The small patch attached here adds support for an -mattr option to the
>>>>> gold plugin and to llvm-lto. This allows the caller to specify details
>>>>> of the subtarget architecture, like +aes, or +ssse3 on x86.  Note that
>>>>> this required a change to the include/llvm-c/lto.h interface: it adds
>>>>> a function lto_codegen_set_attr and it increments the version of the
>>>>> interface.
>>>>>
>>>>> Please let me know what you think.
>>>>
>>>>
>>>> The LTO API already has:
>>>>     lto_codegen_debug_option(lto_codegen_t, const char *);
>>>>
>>>> Could you support -mattr options using the existing API like?:
>>>>      lto_codegen_debug_option(cg, “-mattr=+sse3”);
>>>
>>>
>>> Absolutely not. That API has the word "debug" in its name because it is
>>> strictly reserved for debugging libLTO. That means that it is not part of
>>> libLTO's public interface.
>>
>> It is in the public header<llvm-c/lto.h>.  How can it not be part of the
>> public interface?
>>
>>
>>> The reason is that libLTO has a strong ABI requirement. Once we have a
>>> user using libLTO, we can never ever break them at the ABI level. The
>>> intention is that this exposed interface is something small that we can
>>> audit, and that we can change LLVM's innards without disturbing libLTO.
>>>
>>> Once you allow passing arbitrary flags across, every single flag
>>> everywhere in every llvm library becomes part of the permanent ABI that can
>>> never be changed.
>>
>> That boat has already sailed.
>
>
> No. You may have shipped a product you thought was ABI stable, but you are
> mistaken.
>
>
>   The darwin linker accepts “-mllvm foo” and calls
> lto_codegen_debug_option(xx, “foo”).
>
> Realize that flags are currently whole-process globals. LLVM is a library,
> and there is no place for such globals (consider a program that uses a
> graphics library and an audio library, both libraries are accelerated using
> llvm under the hood. They can not both set these globals). The direction
> we're headed is one where flags in the llvm libraries get transformed into
> APIs and non-global state objects while the flags themselves get shunted
> outwards towards the tools. Ultimately "-mattr" will be a flag in
> tools/llc/llc.cpp not inside the llvm library.
>
> What then?
>
> If you want to maintain backwards compatibility, I suggest you find a set of
> "mllvm" flags you want to continue to support, then add a new string-based
> API for them to libLTO and have libLTO parse those flags and set llvm's
> internal state.
>
> And no, you can't use lto_codegen_debug_option for that purpose. That
> function is for passing arbitrary flags to debug libLTO with. What this
> means is that you've already bought into ABI instability and the linker
> you've already shipped will not be compatible with some future version of
> libLTO.
>
>
>  It has been used with things like -mcpu=blah to work around that
> information not being recorded in the bitcode files.
>>
>>
>> I understand that a string interface by-passes any sort of static
>> analysis.
>
>
> I have no problem with a string interface. I do have a problem with putting
> the entirety of llvm libraries under ABI lock.
>
>
>> Isn’t the long term goal that info like +sse3 be recorded in bit code
>> files so that these options are not needed at link time?  If so, then the
>> proposed API is a short term work around.  My point is that we already have
>> a hack to pass arbitrary option strings.  Why have two?
>
>
> If the plan was to bake arbitrary strings into bitcode and call
> cl::ParseCommandLineOptions then we need a new plan. However, I see no
> problem with baking into the bitcode file the same strings that the backend
> .td files use ("sse3").
>
> I don't even mind if you want to make the string "mattr=+sse3", the only
> requirement is that it needs to be parsed by libLTO itself or something
> that's aware of the ABI stability guarantee.
>

This patch is currently the only short-term way I know to get these
attributes down into LTO, and this is necessary for me to get some
large LTO builds to work. (That is, the only way other than the debug
interface, which seems to have strong opposition on this list). LTO is
currently broken for builds that use special attributes to compile
some files.  Is this patch acceptable for fixing that breakage?

Thanks,

Tom