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

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

on 2024/2/21 15:19, Michael Meissner wrote:
> On Tue, Feb 20, 2024 at 06:35:34PM +0800, Kewen.Lin wrote:
>> Hi Mike,
>>
>> Sorry for late reply (just back from vacation).
>>
>> on 2024/2/8 03:58, Michael Meissner wrote:
>>> On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote:
>>>> on 2024/2/6 14:01, Michael Meissner wrote:
>>>> 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.
>>>
>>> PROCESSOR_DEFAULT can be PROCESSOR_FUTURE if somebody configures GCC with
>>> --with-cpu=future.  While in general it shouldn't occur, it is helpful to
>>> consider all of the corner cases.
>>
>> But it sounds not true, I think you meant TARGET_CPU_DEFAULT instead?
>>
>> On one local ppc64le machine I tried to configure with --with-cpu=power10,
>> I got {,OPTION_}TARGET_CPU_DEFAULT "power10" but PROCESSOR_DEFAULT is still
>> PROCESSOR_POWER7 (PROCESSOR_DEFAULT64 is PROCESSOR_POWER8).  I think these
>> PROCESSOR_DEFAULT{,64} are defined by various headers:
> 
> Yes, I was mistaken.  You are correct TARGET_CPU_DEFAULT is set.  I will change
> the comments.

Thanks!

> 
>> gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7
>> gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7
>> gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7
>> gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7
>> gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER8
>> gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8
>> gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT  PROCESSOR_PPC7400
>> gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT64  PROCESSOR_POWER4
>> gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7450
>> gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8
>> gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7
>> gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8
>> gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT   PROCESSOR_PPC603
>> gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT64 PROCESSOR_RS64A
>> gcc/config/rs6000/vxworks.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC604
>>
>> , and they are unlikely to be updated later, no?
>>
>> btw, the given --with-cpu=future will make cpu_index never negative so
>>
>>   ...
>>   else if (cpu_index >= 0)
>>     rs6000_tune_index = tune_index = cpu_index;
>>   else
>>     ... 
>>
>> so there is no chance to enter "else" arm, that is, that arm only takes
>> effect when no cpu/tune is given (neither -m{cpu,tune} nor --with-cpu=).
> 
> Note, this is existing code.  I didn't modify it.  If we want to change it, we
> should do it as another patch.

Yes, I agree.  Just to clarify, I didn't suggest changing it but instead
suggested almost keeping them, since we don't need any changes in "else"
arm, so instead of updating in arms "if" and "else if" for "future cpu type",
it seems a bit more clear to just check it after this, ie.:

----

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;

----

, copied from previous comment: https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643681.html

BR,
Kewen