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

[Akira Hatanaka]

Thanks. I'll split the patch into two parts (the NFC refactoring part and
the rest) and commit them.

On Wed, May 6, 2015 at 2:54 PM, Eric Christopher <echristo at gmail.com> wrote:

> 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/d404281b/attachment.html>