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

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

Hi Iain,

Thanks again for your help!!

on 2022/9/29 16:16, Iain Sandoe wrote:
> Hi Kewen,
> 
> thanks for looking at this!
> (I suspect it would also affect a 32b linux host with a 64b multilib)
> 

Quite reasonable suspicion.

> quite likely powerpc-darwin is the only 32b ppc host in regular testing.
> 
[...snip...]
>>
>> I'm testing the attached diff which can be applied on top of the previous proposed patch
>> on ppc64 and ppc64le, could you help to test it can fix the issue?
> 
> It does work on a cross from x86_64-darwin => powerpc-darwin, I can also do compile-only
> tests there with a dummy board and the new tests pass with one minor tweak as described
> below.
> 

Nice!  How blind I was, I should have searched for "requires.*PowerPC64".

> full regstrap on the G5 will take a day or so .. but I’ll do the C target tests first to get a heads up.
> 

Thanks!  I think the C target tests is enough for now.  I just refined the patch by
addressing Segher's review comments and some other adjustments, I'm going to test it
on ppc64/ppc64le/aix first, if everything goes well, I'll ask your help for a full
regstrap on the new version.

> ====
> 
> OK. So one small wrinkle, 
> 
> Darwin already has 
> 
>   if (TARGET_64BIT && ! TARGET_POWERPC64)
>     {
>       rs6000_isa_flags |= OPTION_MASK_POWERPC64;
>       warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64");
>     }
> 
> in darwin_rs6000_override_options()
> 
> Which means that we do not report an error, but a warning, and then we force 64b on (taking
> the user’s intention to be specified by the explicit ‘-m64’).
> 
> If there’s a strong feeling that this should really be an error, then I could make that change and
> see what fallout results.

IMHO it's fine to leave it unchanged, aix also follows the same idea emitting warning
instead of error, there are probably some actual user cases relying on this behavior,
changing it can affect them.  Thanks for bringing this up anyway!

> 
> the patch below amends the test expectations to include Darwin with the warning it currently
> reports.

Will incorporate!  Thanks agian!

BR,
Kewen