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

Hi Duncan,

Been thinking about this a bit and a few comments/questions. I may have
misunderstood some things in your mail though so please feel free to
explain at me :)

> 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.
>
I think there are a few options/backend communications that
aren't/haven't
gone this way yet that are probably worth considering:

MCTargetOptions/TargetOptions: Basically a grab bag of functionality, some
of which is duplicated via attributes and some that's not. I think at least
some of these should be replaced by module flags, e.g. ABI.

Random backend flags: Anything for debugging.

I'm thinking things that are either set as Init(true/false) and affect
things at a global level and not just the function level.

> They look like this in assembly:
>
>     attributes default = {
"no-frame-pointer-elim"="false" }
>
>
So, how do you see module merging and defaults working? (Yes, there were
testcases, but let's go with prose here. I found the testcases a bit hard
to reason.)

> 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.
>
Ick. Pretty severe limitation? I.e. how would it work to test general
attributes on a function during code gen?

>   - 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.
>
Hrm. This seems like a pretty severe limitation? Anything come to mind in
practice.

>   - `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.
>
Sure.

>   - 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.
>
>
Seems reasonable.

> Thoughts?  Other ideas for solving the `llc` problem?
>
>
I think this is a good start, I think I'd like to worry about some of the
other issues in advance before we start incrementally changing things
though. (Also, I really have no other ideas :)

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150224/8a94e6d3/attachment.html>

> On 2015-Feb-24, at 13:25, Eric Christopher <echristo at gmail.com>
wrote:
> 
> Hi Duncan,
> 
> Been thinking about this a bit and a few comments/questions. I may have
misunderstood some things in your mail though so please feel free to explain at
me :)
> 
> 
> 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.
> 
> I think there are a few options/backend communications that
aren't/haven't gone this way yet that are probably worth considering:
> 
> MCTargetOptions/TargetOptions: Basically a grab bag of functionality, some
of which is duplicated via attributes and some that's not. I think at least
some of these should be replaced by module flags, e.g. ABI.
> 
> Random backend flags: Anything for debugging.
> 
> I'm thinking things that are either set as Init(true/false) and affect
things at a global level and not just the function level.
(I think my lib/Linker comment was unclear.  See below.)

Not at all trying to say that *everything* should be a function
attribute; global-level stuff should absolutely be module flags.

I'm just talking about infrastructure for the things that *are*
function-level.
> 
> They look like this in assembly:
> 
>     attributes default = {
"no-frame-pointer-elim"="false" }
> 
> 
> So, how do you see module merging and defaults working? (Yes, there were
testcases, but let's go with prose here. I found the testcases a bit hard to
reason.)
This is where my lib/Linker comment applies:
>> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
>> defaults should be pushed down as explicit function attributes.
^ This is how I see module merging and defaults working: push the
defaults down to explicit function attributes.  So there wouldn't
be any default function attributes in the output of `llvm-link`.
This means that `llc` will still have trouble overriding attributes
in the output of merged modules -- but at least it can handle the
output of `clang` without trouble.  In the future we could try to
be more intelligent about merged modules, and keep common options
in the default set.
>  
> 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.
> 
> Ick. Pretty severe limitation? I.e. how would it work to test general
attributes on a function during code gen?
As long as everyone calls `Function::hasFnAttribute()`, there's no
problem.  This proposal basically turns it into a bug to access
them directly; you need to go through `Function::hasFnAttribute()`
to get the right answer.  (Not sure if there's a better way?)
>  
>   - 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.
> 
> Hrm. This seems like a pretty severe limitation? Anything come to mind in
practice.
In the `Attribute` format, mutually exclusive attributes aren't
related at all (they're not inherently mutually exclusive).  To
make them overridable, we'd need a completely new design for
enum attributes.

As a result, this proposal only improves `llc` for string-based
attributes.  I don't see that as a problem... the string-based
attributes are more flexible anyway.  Maybe `Module` should only
allow `setDefaultFnAttribute()` for string attributes though?

(Some more context on why enum attributes can't really be
overridden.  This isn't just a problem for enum attributes that
are mutually exclusive.  Consider:

    attributes defaults = { noreturn }

Besides being somewhat insane, there's no `return` attribute,
so you can't really override it.  I suppose one idea would be to
explicitly mark a function `~noreturn` or something:

    define void @foo() ~noreturn { ; Ignore module default noreturn.

Not sure if this direction is a good one though.)
>  
>   - `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.
> 
> Sure.
>  
>   - 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.
> 
> 
> Seems reasonable.
>  
> Thoughts?  Other ideas for solving the `llc` problem?
> 
> 
> I think this is a good start, I think I'd like to worry about some of
the other issues in advance before we start incrementally changing things
though. (Also, I really have no other ideas :)
> 

So Akira had an idea at the end of last week that I don't
think made it onto the list, and it's worth considering as an
alternative:

Add a bit to attributes indicating whether or not they're
overridable (i.e., they're overridable if they're target
defaults; they're not overridable if they've been explicitly
specified somehow).

Here's some straw-man syntax:

    attributes #0 = { noreturn ssp? "att1"="1"
"att2"="2"? }

Where:

  - `noreturn` and `"att1"="1"` are required.
  - `ssp` and `"att2"="2"` can be overridden (e.g., by
`llc`).

(Alternately, but equivalently:

    attributes #0 = { noreturn! ssp "att1"="1"!
"att2"="2" }

I like this syntax better, but it would cause more churn, and
`!` is so far reserved for metadata.)

Whatever the syntax, the idea is: `llc` resets/deletes
attributes on every function to match what's specified on the
command-line.  In the above example, functions with attribute
set #0 could have `ssp` and `"att2"` overridden via the `llc`
command-line, but not `noreturn` and `"att1"`.

To compare it to my proposal:

  - Storing a default attribute set (my proposal) makes it
    easier to look at and set the defaults.  Applying `llc`
    command-line options is easy, too -- just override the
    default attribute set on the module -- although it doesn't
    really work on the output of `lib/Linker`.
  - Storing a bit on each attribute (Akira's proposal) handles
    more cases.  Nothing needs to be done in `lib/Linker`
    (`llc` is able to override the output of `llvm-link`),
    and it doesn't have a disconnect between `hasFnAttribute()`
    and `getAttributes().hasAttribute(FunctionIndex)`.

Awkwardly, having written that out, I'm kind of leaning toward
it myself right now (maybe I'm fickle?) -- it seems to have
fewer limitations.  The main thing I prefer about my proposal
is that it's easier to change the default attributes when
modifying an assembly file by hand, but I suppose we could
write a tool for that?

On Tue, Feb 24, 2015 at 2:53 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2015-Feb-24, at 13:25, Eric Christopher <echristo at
gmail.com> wrote:
> >
> > Hi Duncan,
> >
> > Been thinking about this a bit and a few comments/questions. I may
have
> misunderstood some things in your mail though so please feel free to
> explain at me :)
> >
> >
> > 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.
> >
> > I think there are a few options/backend communications that
> aren't/haven't gone this way yet that are probably worth
considering:
> >
> > MCTargetOptions/TargetOptions: Basically a grab bag of functionality,
> some of which is duplicated via attributes and some that's not. I think
at
> least some of these should be replaced by module flags, e.g. ABI.
> >
> > Random backend flags: Anything for debugging.
> >
> > I'm thinking things that are either set as Init(true/false) and
affect
> things at a global level and not just the function level.
>
> (I think my lib/Linker comment was unclear.  See below.)
>
> Not at all trying to say that *everything* should be a function
> attribute; global-level stuff should absolutely be module flags.
>
> I'm just talking about infrastructure for the things that *are*
> function-level.
>
> >
> > They look like this in assembly:
> >
> >     attributes default = {
"no-frame-pointer-elim"="false" }
> >
> >
> > So, how do you see module merging and defaults working? (Yes, there
were
> testcases, but let's go with prose here. I found the testcases a bit
hard
> to reason.)
>
> This is where my lib/Linker comment applies:
>
> >> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
> >> defaults should be pushed down as explicit function attributes.
>
> ^ This is how I see module merging and defaults working: push the
> defaults down to explicit function attributes.  So there wouldn't
> be any default function attributes in the output of `llvm-link`.
> This means that `llc` will still have trouble overriding attributes
> in the output of merged modules -- but at least it can handle the
> output of `clang` without trouble.  In the future we could try to
> be more intelligent about merged modules, and keep common options
> in the default set.
>
> >
> > 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.
> >
> > Ick. Pretty severe limitation? I.e. how would it work to test general
> attributes on a function during code gen?
>
> As long as everyone calls `Function::hasFnAttribute()`, there's no
> problem.  This proposal basically turns it into a bug to access
> them directly; you need to go through `Function::hasFnAttribute()`
> to get the right answer.  (Not sure if there's a better way?)
>
> >
> >   - 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.
> >
> > Hrm. This seems like a pretty severe limitation? Anything come to mind
> in practice.
>
> In the `Attribute` format, mutually exclusive attributes aren't
> related at all (they're not inherently mutually exclusive).  To
> make them overridable, we'd need a completely new design for
> enum attributes.
>
> As a result, this proposal only improves `llc` for string-based
> attributes.  I don't see that as a problem... the string-based
> attributes are more flexible anyway.  Maybe `Module` should only
> allow `setDefaultFnAttribute()` for string attributes though?
>
> (Some more context on why enum attributes can't really be
> overridden.  This isn't just a problem for enum attributes that
> are mutually exclusive.  Consider:
>
>     attributes defaults = { noreturn }
>
> Besides being somewhat insane, there's no `return` attribute,
> so you can't really override it.  I suppose one idea would be to
> explicitly mark a function `~noreturn` or something:
>
>     define void @foo() ~noreturn { ; Ignore module default noreturn.
>
> Not sure if this direction is a good one though.)
>
> >
> >   - `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.
> >
> > Sure.
> >
> >   - 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.
> >
> >
> > Seems reasonable.
> >
> > Thoughts?  Other ideas for solving the `llc` problem?
> >
> >
> > I think this is a good start, I think I'd like to worry about some
of
> the other issues in advance before we start incrementally changing things
> though. (Also, I really have no other ideas :)
> >
>
>
> So Akira had an idea at the end of last week that I don't
> think made it onto the list, and it's worth considering as an
> alternative:
>
> Add a bit to attributes indicating whether or not they're
> overridable (i.e., they're overridable if they're target
> defaults; they're not overridable if they've been explicitly
> specified somehow).
>
> Here's some straw-man syntax:
>
>     attributes #0 = { noreturn ssp? "att1"="1"
"att2"="2"? }
>
> Where:
>
>   - `noreturn` and `"att1"="1"` are required.
>   - `ssp` and `"att2"="2"` can be overridden (e.g., by
`llc`).
>
> (Alternately, but equivalently:
>
>     attributes #0 = { noreturn! ssp "att1"="1"!
"att2"="2" }
>
> I like this syntax better, but it would cause more churn, and
> `!` is so far reserved for metadata.)
>
> Whatever the syntax, the idea is: `llc` resets/deletes
> attributes on every function to match what's specified on the
> command-line.  In the above example, functions with attribute
> set #0 could have `ssp` and `"att2"` overridden via the `llc`
> command-line, but not `noreturn` and `"att1"`.
>
> To compare it to my proposal:
>
>   - Storing a default attribute set (my proposal) makes it
>     easier to look at and set the defaults.  Applying `llc`
>     command-line options is easy, too -- just override the
>     default attribute set on the module -- although it doesn't
>     really work on the output of `lib/Linker`.
>   - Storing a bit on each attribute (Akira's proposal) handles
>     more cases.  Nothing needs to be done in `lib/Linker`
>     (`llc` is able to override the output of `llvm-link`),
>     and it doesn't have a disconnect between `hasFnAttribute()`
>     and `getAttributes().hasAttribute(FunctionIndex)`.
>
> Awkwardly, having written that out, I'm kind of leaning toward
> it myself right now (maybe I'm fickle?) -- it seems to have
> fewer limitations.  The main thing I prefer about my proposal
> is that it's easier to change the default attributes when
> modifying an assembly file by hand, but I suppose we could
> write a tool for that?
>
I think the tool approach deserves a bit of attention for the original
usecase of trying different target attributes to see if they tickle a bug.
Would it be feasible to have a purpose-built tool `llvm-attr-mutate`
(bikeshed) so you can do `cat foo.bc | llvm-attr-mutate .... | llc`. The
arguments to llvm-attr-mutate could then be made as convenient/powerful as
needed for this debugging task.

-- Sean Silva

> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150224/5408218b/attachment.html>