Hi Peter,
on 2023/8/24 10:07, Peter Bergner wrote:
> 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.
>
Good point! I also noticed this, but I wasn't sure what the future
supported target/OS combinations will be like, what would be common
fundamentally, then I thought maybe we can just leave it for now.
I change my mind now and I agree we can do more.
> 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).
By looking into the uses of function rs6000_pcrel_p, I think we can
just replace it with TARGET_PCREL. Previously we don't require PCREL
unset for any unsupported target/OS, so we need rs6000_pcrel_p() to
ensure it's really supported in those use places, now if we can guarantee
TARGET_PCREL only hold for all places where it's supported, it looks
we can just check TARGET_PCREL only?
Then the code structure can look like:
if (PCREL_SUPPORTED_BY_OS
&& (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
// enable
else if (TARGET_PCREL && DEFAULT_ABI != ABI_ELFv2)
// disable
else if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
// disable
Here, ABI_ELFv2 and CMODEL_MEDIUM checking is current supported
target/OS specific, in future when we have any new supported
target/OS, this part can be factored out as one subtarget specific
checking function/macro.
Does it make sense?
BR,
Kewen
>
> I think then, that should allow simplifying the code in
> rs6000_option_override_internal.
>
> Thoughts?
>
>
> Peter
>
>
