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
