Re: [PATCH] rs6000: Rework option -mpowerpc64 handling [PR106680]

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

on 2022/9/30 01:11, Segher Boessenkool wrote:
> On Thu, Sep 29, 2022 at 01:45:16PM +0800, Kewen.Lin wrote:
>> I found this flag is mainly related to tune setting and spotted that we have some code
>> for tune setting when no explicit cpu is given. 
>>
>> ...
>>
>>   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;
>> 	  }
>>     }
> 
> Ah cool, that needs fixing yes.
> 
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -3702,7 +3702,7 @@ rs6000_option_override_internal (bool global_init_p)
>>        else
>>  	{
>>  	  /* PowerPC 64-bit LE requires at least ISA 2.07.  */
>> -	  const char *default_cpu = (!TARGET_POWERPC64
>> +	  const char *default_cpu = (!TARGET_POWERPC64 && TARGET_32BIT
>>  				     ? "powerpc"
>>  				     : (BYTES_BIG_ENDIAN
>>  					? "powerpc64"
> 
> ... but not like that.  If this snippet should happen later just move it
> later.  Or introduce a new variable to make the control flow less
> confused.  Or something else.  But don't make the code more complex,
> introducing more special cases like this.

Agree, the diff was mainly to check if it's the root cause.  I think we
need to place TARGET_POWERPC64 enablement for 64 bit before this hunk,
I've adjusted it in the new version, will post it once it's full tested.

> 
>> +#ifdef OS_MISSING_POWERPC64
>> +      else if (OS_MISSING_POWERPC64)
>> +	/* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which
>> +	   miss powerpc64 support, so disable it.  */
>> +	rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
>> +#endif
> 
> All silent stuff is always bad.
> 

OK, with more testings for replacing warning instead of silently disablement
I noticed that some disablement is needed, one typical case is -m32 compilation
on ppc64, we have OPTION_MASK_POWERPC64 on from TARGET_DEFAULT which is used
for initialization (It makes sense to have it on in TARGET_DEFAULT because
of it's 64 bit cpu).  And -m32 compilation matches OS_MISSING_POWERPC64
(!TARGET_64BIT), so it's the case that we have an implicit OPTION_MASK_POWERPC64
on and OS_MISSING_POWERPC64 holds, but it's unexpected not to disable it but
warn it.

BR,
Kewen

> If things are done well, we will end up with *less* code than what we
> had before, not more!
> 
> 
> Segher