Duncan P. N. Exon Smith
2015-Feb-13 00:00 UTC
[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 > >
Jim Grosbach
2015-Feb-13 00:02 UTC
[LLVMdev] [RFC] Storing default function attributes on the module
> 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 >> >> >
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