Re: Repost [PATCH 1/6] Add -mcpu=future

["Kewen.Lin" <linkw@linux.ibm.com>]

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