Jim Grosbach
2015-Mar-10 01:28 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> On Mar 5, 2015, at 1:01 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > >> >> 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.llc should be able to override the default values for options without mutating anything that’s explicitly specified at a function (or module) level. For example, if I have a .ll file with specialized functions with different CPUs specified, I want to be able to recompile that file with -mcpu= on the llc command line in such a way that those functions don’t get changed, but any functions that don’t have an explicit CPU on them do. Consider trying to test something like function multi versioning. -Jim -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150309/f289a489/attachment.html>
Rafael Espíndola
2015-Mar-11 20:06 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> 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. > > > > llc should be able to override the default values for options without > mutating anything that’s explicitly specified at a function (or module) > level. > > For example, if I have a .ll file with specialized functions with different > CPUs specified, I want to be able to recompile that file with -mcpu= on the > llc command line in such a way that those functions don’t get changed, but > any functions that don’t have an explicit CPU on them do. Consider trying to > test something like function multi versioning.Sure, but that can be implemented with 4, no? Something like: * In llc, if given -mcpu: * Iterate over all functions: * If a function has a an explicit cpu, leave it alone. * If a function has no explicit cpu, set it to the -mcpu command line value Cheers, Rafael
Eric Christopher
2015-Mar-11 20:10 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
On Wed, Mar 11, 2015 at 1:08 PM Rafael Espíndola <rafael.espindola at gmail.com> wrote:> > 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. > > > > > > > > llc should be able to override the default values for options without > > mutating anything that’s explicitly specified at a function (or module) > > level. > > > > For example, if I have a .ll file with specialized functions with > different > > CPUs specified, I want to be able to recompile that file with -mcpu= on > the > > llc command line in such a way that those functions don’t get changed, > but > > any functions that don’t have an explicit CPU on them do. Consider > trying to > > test something like function multi versioning. > > Sure, but that can be implemented with 4, no? Something like: > > * In llc, if given -mcpu: > * Iterate over all functions: > * If a function has a an explicit cpu, leave it alone. > * If a function has no explicit cpu, set it to the -mcpu command line > value > >The problem with some of this is that we could see a lot of default cpus on functions. I.e. it may be set most of the time. That's still up in the air though. -eric> Cheers, > Rafael > > _______________________________________________ > 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/20150311/f7ae0ddc/attachment.html>