<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 13, 2015 at 11:13 AM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Duncan<br>
<br>
The first patch is general goodness and I think should be committed now.<br>
<br>
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.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Pete<br>
<div class="HOEnZb"><div class="h5">> On Feb 12, 2015, at 4:02 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<br>
><br>
><br>
>> On Feb 12, 2015, at 4:00 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> +grosbach<br>
>><br>
>>> On 2015-Feb-12, at 14:45, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
>>><br>
>>> Are llc command line options all that critical? It's not that hard to edit the attributes directly or remove them with sed.<br>
>><br>
>> Maybe Jim can speak to this one better than I can, but the workflow<br>
>> I've heard concerns about is:<br>
>><br>
>> - Got a codegen bug (PR or whatever).<br>
>> - Want to fiddle with codegen options in `llc`, to see which ones<br>
>> affect the bug and which don't.<br>
>> - Don't want command-line options to influence attributes that<br>
>> were specified explicitly.<br>
>> - Obviously want to influence the others.<br>
>><br>
>> Sure, `sed` could do this, but it's manual and fairly error-prone,<br>
>> and would have a pretty tough time figuring out which attributes<br>
>> are there because they're target defaults vs. specified in the<br>
>> source.<br>
><br>
> 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.<br>
><br>
>><br>
>>> The less codegen depends on llc command line flags, the better, IMO.<br>
>><br>
>> This doesn't make sense to me. The only command-line flags in `llc`<br>
>> are codegen options... so we remove all `llc` flags?<br>
>><br>
>> I'm not suggesting we push more command-line flags through CodeGen;<br>
>> I just don't want `llc` to *break*. (IMO, `llc` could/should just<br>
>> modify the module-level defaults I've added here, but that's not<br>
>> part of this proposal since there seem to be a ton of weird issues<br>
>> with command-line options and I don't really want to get involved.<br>
>> Just looking to maintain current functionality.)<br>
>><br>
>>> On Wed, Feb 11, 2015 at 11:34 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>> As we encode more CodeGen and target-specific options in bitcode to<br>
>>> support LTO, we risk crippling `llc` as a debugging tool. In<br>
>>> particular, `llc` command-line options are generally ignored when a<br>
>>> function has an attribute set explicitly, but the plan of record is for<br>
>>> `clang` to explicitly encode all (or most) CodeGen options -- even the<br>
>>> target defaults.<br>
>>><br>
>>> Changing `clang` to store target defaults on the module will allow us to<br>
>>> continue to override them when running `llc`. The right precedence<br>
>>> would be:<br>
>>><br>
>>> 1. Explicit attributes set on the function.<br>
>>> 2. `llc` command-line options.<br>
>>> 3. Default function attributes stored on the module.<br>
>>><br>
>>> (Outside of `llc`, skip step 2.)<br>
>>><br>
>>> In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),<br>
>>> defaults should be pushed down as explicit function attributes.<br>
>>><br>
>>> Default function-level attributes<br>
>>> =================================<br>
>>><br>
>>> I've attached patches with a reference implementation.<br>
>>><br>
>>> - 0001: Canonicalize access to function attributes to use<br>
>>> `getFnAttribute()` and `hasFnAttribute()`. (This seems like a nice<br>
>>> cleanup regardless?)<br>
>>> - 0002: Add the feature.<br>
>>> - 0003: Use it in `clang` for function attributes based solely on<br>
>>> `CodeGenOptions`.<br>
>>><br>
>>> They look like this in assembly:<br>
>>><br>
>>> attributes default = { "no-frame-pointer-elim"="false" }<br>
>>><br>
>>> Limitations<br>
>>> ===========<br>
>>><br>
>>> There are a few limitations with this approach (at least, with my<br>
>>> reference implementation).<br>
>>><br>
>>> - `Function::getAttributes()` only reflects the explicitly specified<br>
>>> attributes, skipping those set as module defaults.<br>
>>> - If an enum attribute is set as a default, there's no way for a<br>
>>> function-attribute to override it. In practice, we could avoid the<br>
>>> feature for enum attributes.<br>
>>> - `CallSite` instructions store function-level attributes, but don't<br>
>>> forward to the module-level defaults. There are places (like the<br>
>>> calls to `EmitUnaryFloatFnCall()` in `-simplify-libcalls`) where we<br>
>>> use the callee function attributes to set the call site attributes.<br>
>>> In practice, we could avoid the feature for attributes that are<br>
>>> meaningful for call sites.<br>
>>> - Intrinsics' attributes are independent of `CodeGenOptions`, and set<br>
>>> via `Instrinsic::getAttributes()`. With this change they'd inherit<br>
>>> the default attributes like other functions. Is this a problem?<br>
>>> If so, we can add a flag on `Function` that inhibits forwarding to<br>
>>> the defaults.<br>
>>><br>
>>> Thoughts? Other ideas for solving the `llc` problem?<br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> LLVM Developers mailing list<br>
>>> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>>><br>
>>><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</div></div></blockquote></div><br></div></div>