[PATCH] Let llc and opt override "-target-cpu" and "-target-features" using command line options "-mcpu" and "-mattr"

[Eric Christopher]

Cool deal. Feel free to commit after.

Thanks for the work here.

-eric

On Wed, May 6, 2015 at 2:53 PM Akira Hatanaka <ahatanak at gmail.com> wrote:

> On Wed, May 6, 2015 at 1:55 PM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>>
>>
>> On Wed, May 6, 2015 at 1:51 PM Akira Hatanaka <ahatanak at gmail.com> wrote:
>>
>>> In http://reviews.llvm.org/D9537#167157, @echristo wrote:
>>>
>>> > I agree, no TargetOptions for this. I don't necessarily agree with
>>> rewriting, but it seems to be a small enough change and will work for what
>>> the overrides are meant to be for. Some discipline in reviewing testcases
>>> to make sure that a lot of overrides aren't there for incoming testcases is
>>> going to be necessary for clean tests. One inline comment which basically
>>> sums up to "why not two functions"? As long as you have a reasonable answer
>>> go ahead.
>>>
>>>
>>> I'm not sure if I understood your comment about testcases. Do you mean
>>> people shouldn't duplicate functions in test cases just to test different
>>> target-cpu and target-feature strings but instead should use -mcpu and
>>> -mattrs (which I think makes sense)? Should the one inline comment go into
>>> the test cases I added?
>>>
>>>
>>>
>> My comment about testcases is that someone shouldn't have:
>>
>> ; RUN: llc -march=foo
>>
>> as the only run line with a function with:
>>
>> target-cpu="notfoo"
>>
>> when they could just change the arch on the function to say "foo" rather
>> than "notfoo".
>>
>>
>
> OK, I agree. In that case, people should be advised to remove the command
> line option and change the "target-cpu" attribute or just remove the
> "target-cpu" attribute.
>
> I'll add comments to the test cases in this patch to make it clear what
> they are testing.
>
> ================
>>> Comment at: include/llvm/CodeGen/CommandFlags.h:268
>>> @@ -262,1 +267,3 @@
>>>
>>> +static inline std::pair<std::string, std::string> getCPUFeaturesStr() {
>>> +  std::string CPUStr;
>>> ----------------
>>> echristo wrote:
>>> > Why a pair?
>>> Is it better to split this into two functions getCPUStr and
>>> getFeaturesStr? Or you meant the two strings (cpu and features strings)
>>> should be passed by reference?
>>>
>>>
>> I meant split into two functions, any particular reason?
>>
>>
> No particular reason. I'll split the function into two.
>
>
>> -eric
>>
>>
>>> http://reviews.llvm.org/D9537
>>>
>>> EMAIL PREFERENCES
>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150506/a3c41bf2/attachment.html>