Pete Cooper
2015-Feb-13 19:13 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
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
Duncan P. N. Exon Smith
2015-Feb-13 23:54 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> On 2015-Feb-13, at 11:13, Pete Cooper <peter_cooper at apple.com> wrote: > > Hi Duncan > > The first patch is general goodness and I think should be committed now.That's what I thought. I'll start committing.> > 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.Cool. I figure I'll sit on them at least the next weekly mail goes out, just in case someone finds a problem with the direction.> 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 >
Sean Silva
2015-Feb-19 00:19 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
On Fri, Feb 13, 2015 at 11:13 AM, Pete Cooper <peter_cooper at apple.com> wrote:> 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. >I'm wondering if we could wire the LLC options to just change what Duncan's OP called "default module options"? If we have a way to transport these settings to the backend in the module as data, having a path in the code seems redundant; better to canonicalize on always getting this information to the backend through the module. As a bonus, that helps with Reid's concern too, I think. -- Sean Silva> > 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 > > > _______________________________________________ > 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/20150218/573b72f3/attachment.html>
Pete Cooper
2015-Feb-19 00:22 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> On Feb 18, 2015, at 4:19 PM, Sean Silva <chisophugis at gmail.com> wrote: > > > > On Fri, Feb 13, 2015 at 11:13 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote: > 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. > > I'm wondering if we could wire the LLC options to just change what Duncan's OP called "default module options"? If we have a way to transport these settings to the backend in the module as data, having a path in the code seems redundant; better to canonicalize on always getting this information to the backend through the module. As a bonus, that helps with Reid's concern too, I think.Sounds good to me. It would be really nice to have a single place to look to see the default options, and you’ll see them every time you dump the module too which is a bonus. Pete> > -- Sean Silva > > > Thanks, > Pete > > On Feb 12, 2015, at 4:02 PM, Jim Grosbach <grosbach at apple.com <mailto:grosbach at apple.com>> wrote: > > > > > >> On Feb 12, 2015, at 4:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote: > >> > >> +grosbach > >> > >>> On 2015-Feb-12, at 14:45, Reid Kleckner <rnk at google.com <mailto: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 <mailto: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 <mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/> > >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev <http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev> > >>> > >>> > >> > > > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev <http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev> > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev <http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150218/f01c3bd3/attachment.html>