Sean Silva
2015-Feb-24 23:18 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
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>
Eric Christopher
2015-Feb-26 19:34 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
On Tue, Feb 24, 2015 at 3:18 PM Sean Silva <chisophugis at gmail.com> wrote:> 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. > >> >>FWIW the tool approach is what we were coming up with in the first place :) Which would obviate the need for handling the command line options at all of course. -eric -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150226/a2717e37/attachment.html>
Duncan P. N. Exon Smith
2015-Mar-05 21:01 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> On 2015 Feb 26, at 11:34, Eric Christopher <echristo at gmail.com> wrote: > > > > On Tue, Feb 24, 2015 at 3:18 PM Sean Silva <chisophugis at gmail.com> wrote: > 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. > > > FWIW the tool approach is what we were coming up with in the first place :) > > Which would obviate the need for handling the command line options at all of course. >I've done some more thinking about this. 1. The module-level default attribute approach (what I started this thread with) cleans up assembly files and provides a clean hook for mutating the "defaults" from `llc`. However, it doesn't cleanly fit into the current semantic model for attributes, so you get weird inconsistencies depending on how you query them. 2. Flipping a bit to indicate whether something was the "default" solves the various technical problems. However, overriding the attributes requires walking all of the IR to rewrite them, and the assembly syntax readability will get worse, not better. 3. Using a separate tool allows you to override the attributes arbitrarily, but doesn't distinguish between "default" and explicit attributes. Furthermore, if the primary use for the tool is: llvm-attr-mutate -o - t.bc -attr target-cpu=proc123 | llc then I'm not sure we're gaining anything. In particular, having `llc` mutate the IR when you say: llc <t.bc -target-cpu=proc123 is a far cleaner interface. Not to say that a tool to mutate attributes isn't a great idea -- I think it might be -- just that it's not a great solution for *this* problem. 4. Having `llc` mutate the IR itself -- the obvious solution, which Akira posted a patch for a few months ago -- does the job just as well as `llvm-attr-mutate` but with a much cleaner interface. It fails to distinguish between target defaults and explicit attributes, but when combined with `llvm-extract`, it gives you full control over the codegen options for each function. Remind me again why we don't just do #4? It seems like the simplest way to keep `llc` viable in the short term.