llvm dev - Feb 2015 - [LLVMdev] [RFC] Storing default function attributes on the module

+grosbach
> On 2015-Feb-12, at 14:45, Reid Kleckner <rnk at google.com> wrote:
> 
> Are llc command line options all that critical? It's not that hard to
edit the attributes directly or remove them with sed.
Maybe Jim can speak to this one better than I can, but the workflow
I've heard concerns about is:

  - Got a codegen bug (PR or whatever).
  - Want to fiddle with codegen options in `llc`, to see which ones
    affect the bug and which don't.
  - Don't want command-line options to influence attributes that
    were specified explicitly.
  - Obviously want to influence the others.

Sure, `sed` could do this, but it's manual and fairly error-prone,
and would have a pretty tough time figuring out which attributes
are there because they're target defaults vs. specified in the
source.
> The less codegen depends on llc command line flags, the better, IMO.
This doesn't make sense to me.  The only command-line flags in `llc`
are codegen options... so we remove all `llc` flags?

I'm not suggesting we push more command-line flags through CodeGen;
I just don't want `llc` to *break*.  (IMO, `llc` could/should just
modify the module-level defaults I've added here, but that's not
part of this proposal since there seem to be a ton of weird issues
with command-line options and I don't really want to get involved.
Just looking to maintain current functionality.)
> On Wed, Feb 11, 2015 at 11:34 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> As we encode more CodeGen and target-specific options in bitcode to
> support LTO, we risk crippling `llc` as a debugging tool.  In
> particular, `llc` command-line options are generally ignored when a
> function has an attribute set explicitly, but the plan of record is for
> `clang` to explicitly encode all (or most) CodeGen options -- even the
> target defaults.
> 
> Changing `clang` to store target defaults on the module will allow us to
> continue to override them when running `llc`.  The right precedence
> would be:
> 
>   1. Explicit attributes set on the function.
>   2. `llc` command-line options.
>   3. Default function attributes stored on the module.
> 
> (Outside of `llc`, skip step 2.)
> 
> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
> defaults should be pushed down as explicit function attributes.
> 
> Default function-level attributes
> ================================> 
> I've attached patches with a reference implementation.
> 
>   - 0001: Canonicalize access to function attributes to use
>     `getFnAttribute()` and `hasFnAttribute()`.  (This seems like a nice
>     cleanup regardless?)
>   - 0002: Add the feature.
>   - 0003: Use it in `clang` for function attributes based solely on
>     `CodeGenOptions`.
> 
> They look like this in assembly:
> 
>     attributes default = {
"no-frame-pointer-elim"="false" }
> 
> Limitations
> ==========> 
> There are a few limitations with this approach (at least, with my
> reference implementation).
> 
>   - `Function::getAttributes()` only reflects the explicitly specified
>     attributes, skipping those set as module defaults.
>   - If an enum attribute is set as a default, there's no way for a
>     function-attribute to override it.  In practice, we could avoid the
>     feature for enum attributes.
>   - `CallSite` instructions store function-level attributes, but don't
>     forward to the module-level defaults.  There are places (like the
>     calls to `EmitUnaryFloatFnCall()` in `-simplify-libcalls`) where we
>     use the callee function attributes to set the call site attributes.
>     In practice, we could avoid the feature for attributes that are
>     meaningful for call sites.
>   - Intrinsics' attributes are independent of `CodeGenOptions`, and set
>     via `Instrinsic::getAttributes()`.  With this change they'd inherit
>     the default attributes like other functions.  Is this a problem?
>     If so, we can add a flag on `Function` that inhibits forwarding to
>     the defaults.
> 
> Thoughts?  Other ideas for solving the `llc` problem?
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
>

> On Feb 12, 2015, at 4:00 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 
> +grosbach
> 
>> On 2015-Feb-12, at 14:45, Reid Kleckner <rnk at google.com>
wrote:
>> 
>> Are llc command line options all that critical? It's not that hard
to edit the attributes directly or remove them with sed.
> 
> Maybe Jim can speak to this one better than I can, but the workflow
> I've heard concerns about is:
> 
>  - Got a codegen bug (PR or whatever).
>  - Want to fiddle with codegen options in `llc`, to see which ones
>    affect the bug and which don't.
>  - Don't want command-line options to influence attributes that
>    were specified explicitly.
>  - Obviously want to influence the others.
> 
> Sure, `sed` could do this, but it's manual and fairly error-prone,
> and would have a pretty tough time figuring out which attributes
> are there because they're target defaults vs. specified in the
> source.
Yep. Duncan summarized it nicely. Breaking llc’s ability to use these options to
debug problems will be a *very* big usability loss for LLVM backend devs.
> 
>> The less codegen depends on llc command line flags, the better, IMO.
> 
> This doesn't make sense to me.  The only command-line flags in `llc`
> are codegen options... so we remove all `llc` flags?
> 
> I'm not suggesting we push more command-line flags through CodeGen;
> I just don't want `llc` to *break*.  (IMO, `llc` could/should just
> modify the module-level defaults I've added here, but that's not
> part of this proposal since there seem to be a ton of weird issues
> with command-line options and I don't really want to get involved.
> Just looking to maintain current functionality.)
> 
>> On Wed, Feb 11, 2015 at 11:34 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>> As we encode more CodeGen and target-specific options in bitcode to
>> support LTO, we risk crippling `llc` as a debugging tool.  In
>> particular, `llc` command-line options are generally ignored when a
>> function has an attribute set explicitly, but the plan of record is for
>> `clang` to explicitly encode all (or most) CodeGen options -- even the
>> target defaults.
>> 
>> Changing `clang` to store target defaults on the module will allow us
to
>> continue to override them when running `llc`.  The right precedence
>> would be:
>> 
>>  1. Explicit attributes set on the function.
>>  2. `llc` command-line options.
>>  3. Default function attributes stored on the module.
>> 
>> (Outside of `llc`, skip step 2.)
>> 
>> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
>> defaults should be pushed down as explicit function attributes.
>> 
>> Default function-level attributes
>> ================================>> 
>> I've attached patches with a reference implementation.
>> 
>>  - 0001: Canonicalize access to function attributes to use
>>    `getFnAttribute()` and `hasFnAttribute()`.  (This seems like a nice
>>    cleanup regardless?)
>>  - 0002: Add the feature.
>>  - 0003: Use it in `clang` for function attributes based solely on
>>    `CodeGenOptions`.
>> 
>> They look like this in assembly:
>> 
>>    attributes default = {
"no-frame-pointer-elim"="false" }
>> 
>> Limitations
>> ==========>> 
>> There are a few limitations with this approach (at least, with my
>> reference implementation).
>> 
>>  - `Function::getAttributes()` only reflects the explicitly specified
>>    attributes, skipping those set as module defaults.
>>  - If an enum attribute is set as a default, there's no way for a
>>    function-attribute to override it.  In practice, we could avoid the
>>    feature for enum attributes.
>>  - `CallSite` instructions store function-level attributes, but
don't
>>    forward to the module-level defaults.  There are places (like the
>>    calls to `EmitUnaryFloatFnCall()` in `-simplify-libcalls`) where we
>>    use the callee function attributes to set the call site attributes.
>>    In practice, we could avoid the feature for attributes that are
>>    meaningful for call sites.
>>  - Intrinsics' attributes are independent of `CodeGenOptions`, and
set
>>    via `Instrinsic::getAttributes()`.  With this change they'd
inherit
>>    the default attributes like other functions.  Is this a problem?
>>    If so, we can add a flag on `Function` that inhibits forwarding to
>>    the defaults.
>> 
>> Thoughts?  Other ideas for solving the `llc` problem?
>> 
>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> 
>> 
>

Hi Duncan

The first patch is general goodness and I think should be committed now.

The other 2 LGTM.  Unless anyone fundamentally objects to module attributes, or
has feedback on the patches themselves, then please commit.  I didn’t see any
problems with them.

Thanks,
Pete
> On Feb 12, 2015, at 4:02 PM, Jim Grosbach <grosbach at apple.com>
wrote:
> 
> 
>> On Feb 12, 2015, at 4:00 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
>> 
>> +grosbach
>> 
>>> On 2015-Feb-12, at 14:45, Reid Kleckner <rnk at google.com>
wrote:
>>> 
>>> Are llc command line options all that critical? It's not that
hard to edit the attributes directly or remove them with sed.
>> 
>> Maybe Jim can speak to this one better than I can, but the workflow
>> I've heard concerns about is:
>> 
>> - Got a codegen bug (PR or whatever).
>> - Want to fiddle with codegen options in `llc`, to see which ones
>>   affect the bug and which don't.
>> - Don't want command-line options to influence attributes that
>>   were specified explicitly.
>> - Obviously want to influence the others.
>> 
>> Sure, `sed` could do this, but it's manual and fairly error-prone,
>> and would have a pretty tough time figuring out which attributes
>> are there because they're target defaults vs. specified in the
>> source.
> 
> Yep. Duncan summarized it nicely. Breaking llc’s ability to use these
options to debug problems will be a *very* big usability loss for LLVM backend
devs.
> 
>> 
>>> The less codegen depends on llc command line flags, the better,
IMO.
>> 
>> This doesn't make sense to me.  The only command-line flags in
`llc`
>> are codegen options... so we remove all `llc` flags?
>> 
>> I'm not suggesting we push more command-line flags through CodeGen;
>> I just don't want `llc` to *break*.  (IMO, `llc` could/should just
>> modify the module-level defaults I've added here, but that's
not
>> part of this proposal since there seem to be a ton of weird issues
>> with command-line options and I don't really want to get involved.
>> Just looking to maintain current functionality.)
>> 
>>> On Wed, Feb 11, 2015 at 11:34 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>>> As we encode more CodeGen and target-specific options in bitcode to
>>> support LTO, we risk crippling `llc` as a debugging tool.  In
>>> particular, `llc` command-line options are generally ignored when a
>>> function has an attribute set explicitly, but the plan of record is
for
>>> `clang` to explicitly encode all (or most) CodeGen options -- even
the
>>> target defaults.
>>> 
>>> Changing `clang` to store target defaults on the module will allow
us to
>>> continue to override them when running `llc`.  The right precedence
>>> would be:
>>> 
>>> 1. Explicit attributes set on the function.
>>> 2. `llc` command-line options.
>>> 3. Default function attributes stored on the module.
>>> 
>>> (Outside of `llc`, skip step 2.)
>>> 
>>> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
>>> defaults should be pushed down as explicit function attributes.
>>> 
>>> Default function-level attributes
>>> ================================>>> 
>>> I've attached patches with a reference implementation.
>>> 
>>> - 0001: Canonicalize access to function attributes to use
>>>   `getFnAttribute()` and `hasFnAttribute()`.  (This seems like a
nice
>>>   cleanup regardless?)
>>> - 0002: Add the feature.
>>> - 0003: Use it in `clang` for function attributes based solely on
>>>   `CodeGenOptions`.
>>> 
>>> They look like this in assembly:
>>> 
>>>   attributes default = {
"no-frame-pointer-elim"="false" }
>>> 
>>> Limitations
>>> ==========>>> 
>>> There are a few limitations with this approach (at least, with my
>>> reference implementation).
>>> 
>>> - `Function::getAttributes()` only reflects the explicitly
specified
>>>   attributes, skipping those set as module defaults.
>>> - If an enum attribute is set as a default, there's no way for
a
>>>   function-attribute to override it.  In practice, we could avoid
the
>>>   feature for enum attributes.
>>> - `CallSite` instructions store function-level attributes, but
don't
>>>   forward to the module-level defaults.  There are places (like the
>>>   calls to `EmitUnaryFloatFnCall()` in `-simplify-libcalls`) where
we
>>>   use the callee function attributes to set the call site
attributes.
>>>   In practice, we could avoid the feature for attributes that are
>>>   meaningful for call sites.
>>> - Intrinsics' attributes are independent of `CodeGenOptions`,
and set
>>>   via `Instrinsic::getAttributes()`.  With this change they'd
inherit
>>>   the default attributes like other functions.  Is this a problem?
>>>   If so, we can add a flag on `Function` that inhibits forwarding
to
>>>   the defaults.
>>> 
>>> Thoughts?  Other ideas for solving the `llc` problem?
>>> 
>>> 
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>> 
>>> 
>> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev