on 2024/2/6 14:01, Michael Meissner wrote:
> On Tue, Jan 23, 2024 at 04:44:32PM +0800, Kewen.Lin wrote:
...
>>> diff --git a/gcc/config/rs6000/rs6000-opts.h
>>> b/gcc/config/rs6000/rs6000-opts.h
>>> index 33fd0efc936..25890ae3034 100644
>>> --- a/gcc/config/rs6000/rs6000-opts.h
>>> +++ b/gcc/config/rs6000/rs6000-opts.h
>>> @@ -67,7 +67,9 @@ enum processor_type
>>> PROCESSOR_MPCCORE,
>>> PROCESSOR_CELL,
>>> PROCESSOR_PPCA2,
>>> - PROCESSOR_TITAN
>>> + PROCESSOR_TITAN,
>>> +
>>
>> Nit: unintentional empty line?
>>
>>> + PROCESSOR_FUTURE
>>> };
>
> It was more as a separation. The MPCCORE, CELL, PPCA2, and TITAN are rather
> old processors. I don't recall why we kept them after the POWER<x>.
>
> Logically we should re-order the list and move MPCCORE, etc. earlier, but I
> will delete the blank line in future patches.
Thanks for clarifying, the re-order thing can be done in a separate patch and
in this context one comment line would be better than a blank line. :)
...
>>> + power10 tuning until future tuning is added. */
>>> if (rs6000_tune_index >= 0)
>>> - tune_index = rs6000_tune_index;
>>> + {
>>> + enum processor_type cur_proc
>>> + = processor_target_table[rs6000_tune_index].processor;
>>> +
>>> + if (cur_proc == PROCESSOR_FUTURE)
>>> + {
>>> + static bool issued_future_tune_warning = false;
>>> + if (!issued_future_tune_warning)
>>> + {
>>> + issued_future_tune_warning = true;
>>
>> This seems to ensure we only warn this once, but I noticed that in rs6000/
>> only some OPT_Wpsabi related warnings adopt this way, I wonder if we don't
>> restrict it like this, for a tiny simple case, how many times it would warn?
>
> In a simple case, you would only get the warning once. But if you use
> __attribute__((__target__(...))) or #pragma target ... you might see it more
> than once.
OK, considering we only get this warning once for a simple case, I'm inclined
not to keep a static variable for it, it's the same as what we do currently
for option conflict errors emission. But I'm fine for either.
>>> else
>>> {
>>> - size_t i;
>>> enum processor_type tune_proc
>>> = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT);
>>>
>>> - tune_index = -1;
>>> - for (i = 0; i < ARRAY_SIZE (processor_target_table); i++)
>>> - if (processor_target_table[i].processor == tune_proc)
>>> - {
>>> - tune_index = i;
>>> - break;
>>> - }
>>> + tune_index = rs600_cpu_index_lookup (tune_proc == PROCESSOR_FUTURE
>>> + ? PROCESSOR_POWER10
>>> + : tune_proc);
>>
>> This part looks useless, as tune_proc is impossible to be PROCESSOR_FUTURE.
>
> Well in theory, you could configure the compiler with --with-cpu=future or
> --with-tune=future.
Sorry for the possible confusion here, the "tune_proc" that I referred to is
the variable in the above else branch:
enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 :
PROCESSOR_DEFAULT);
It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a
chance to be PROCESSOR_FUTURE, so the checking "tune_proc == PROCESSOR_FUTURE"
is useless.
That's why I suggested the below flow, it does a final check out of those
checks,
it looks a bit more clear IMHO.
>
>>> }
>>
>> Maybe re-structure the above into:
>>
>> bool explicit_tune = false;
>> if (rs6000_tune_index >= 0)
>> {
>> tune_index = rs6000_tune_index;
>> explicit_tune = true;
>> }
>> else if (cpu_index >= 0)
>> // as before
>> rs6000_tune_index = tune_index = cpu_index;
>> else
>> {
>> //as before
>> ...
>> }
>>
>> // Check tune_index here instead.
>>
>> if (processor_target_table[tune_index].processor == PROCESSOR_FUTURE)
>> {
>> tune_index = rs6000_cpu_index_lookup (PROCESSOR_POWER10);
>> if (explicit_tune)
>> warn ...
>> }
>>
>> // as before
>> rs6000_tune = processor_target_table[tune_index].processor;
>>
>>>
BR,
Kewen
