Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner <bergner@linux.ibm.com>]

On 8/21/23 8:51 PM, Kewen.Lin wrote:
>> The following patch has been bootstrapped and regtested on powerpc64-linux.
> 
> I think we should test this on powerpc64le-linux P8 or P9 (no P10) as well.

That's a good idea!



> I think this should be moved to be with the hunk on PCREL:
> 
>   /* If the ABI has support for PC-relative relocations, enable it by default.
>      This test depends on the sub-target tests above setting the code model to
>      medium for ELF v2 systems.  */
>   if (PCREL_SUPPORTED_BY_OS
>       && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
>     rs6000_isa_flags |= OPTION_MASK_PCREL;
> 
>   /* -mpcrel requires -mcmodel=medium, but we can't check TARGET_CMODEL until
>       after the subtarget override options are done.  */
>   else if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
>     {
>       if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
> 	error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
> 
>       rs6000_isa_flags &= ~OPTION_MASK_PCREL;
>     }

Agreed on the location, but...

Looking at this closer, I don't think I'm happy with the current code.
We seem to have duplicated tests for whether the target supports pcrel
or not in both PCREL_SUPPORTED_BY_OS and rs6000_pcrel_p().  That means
if another target were to add support for pcrel, they'd have to update
multiple locations of the code, and that seems error prone.

I think we should standardize our tests for whether the target/OS
supports pcrel (irrespective of the -mpcrel or -mcmodel=medium options)
and have that in PCREL_SUPPORTED_BY_OS.  Ie, a one-stop-shop for testing
whether the current target/OS can support pcrel.  Then we should modify
rs6000_pcrel_p() use PCREL_SUPPORTED_BY_OS rather than its own
semi-duplicated target/OS tests, plus any other tests for options that
might disqualify the current target/OS from supporting pcrel, when it
normally can (ie, -mmodel != medium for ELFv2).

I think then, that should allow simplifying the code in
rs6000_option_override_internal.

Thoughts?


Peter