on 2023/8/25 11:20, Peter Bergner wrote:
> On 8/24/23 12:56 AM, Kewen.Lin wrote:
>> 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?
>
> I think you're correct on only needing TARGET_PCREL instead of
> rs6000_pcrel_p()
> as long as we correctly disable PCREL on the targets that either don't support
> it or those that due, but don't due to explicit options (ie, -mno-prel or
> ELFv2 and -mcmodel != medium, etc.).
>
>
>
>> 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
>
> I don't like that first conditional expression. The problem is,
> PCREL_SUPPORTED_BY_OS could be true or false for the following
> tests, because it's anded with the explicit option test, and
> sometimes that won't make sense. I think something safer is
> something like:
Assuming the current PCREL_SUPPORTED_BY_OS unchanged, when
PCREL_SUPPORTED_BY_OS is true, all its required conditions are
satisfied, it should be safe. while PCREL_SUPPORTED_BY_OS is
false, it means the given subtarget doesn't support it, or one
or more of conditions below don't hold:
- TARGET_POWER10
- TARGET_PREFIXED
- ELFv2_ABI_CHECK
- TARGET_CMODEL == CMODEL_MEDIUM
The two "else if" check for the last two (abi test also can
check unsupported targets), we have check code for TARGET_PCREL
without TARGET_PREFIXED support, and further Power10, so it
seems safe?
>
> if (PCREL_SUPPORTED_BY_OS)
> {
> /* PCREL on ELFv2 requires -mcmodel=medium. */
> if (DEFAULT_ABI == ABI_ELFv2 && TARGET_CMODEL != CMODEL_MEDIUM)
> error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
I guess your proposal seems to drop CMODEL_MEDIUM (even PREFIX?)
from PCREL_SUPPORTED_BY_OS, to leave it just ABI_ELFv2 & P10? (
otherwise TARGET_CMODEL should be always CMODEL_MEDIUM for
ABI_ELFv2 with the original PCREL_SUPPORTED_BY_OS). And it
seems to make the subtarget specific checking according to the
ABI type further?
I was expecting that when new subtargets need to be supported, we
can move these subtarget specific checkings into macro/function
SUB*TARGET_OVERRIDE_OPTIONS, IMHO the upside is each subtarget
can take care of its own checkings separately. Maybe we can
just move them now (to rs6000_linux64_override_options). And in
the common code, for the cases with zero value PCREL_SUPPORTED_BY_OS,
(assuming PCREL_SUPPORTED_BY_OS just simple as like ABI_ELFv2), we
can emit an invalid error for explicit -mpcrel as you proposed
below. Thoughts?
btw, I was also expecting that we don't implicitly set
OPTION_MASK_PCREL any more for Power10, that is to remove
OPTION_MASK_PCREL from OTHER_POWER10_MASKS.
>
> if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
> rs6000_isa_flags |= OPTION_MASK_PCREL;
> }
> else
> {
> if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0
> && TARGET_PCREL)
> error ("use of %qs is invalid for this target", "-mpcrel");
I like this, it's more clear!
> rs6000_isa_flags &= ~OPTION_MASK_PCREL;
> }
>
> ...although, even the cmodel != medium test above should probably have
> some extra tests to ensure TARGET_PCREL is true (and explicit?) and
> mcmodel != medium before giving an error??? Ie, a ELFv2 P10 compile
> with an implicit -mpcrel and explicit -mcmodel={small,large} probably
> should not error and just silently disable pcrel??? Meaning only
> when we explicitly say -mpcrel -mcmodel={small,large} should we give
> that error. Thoughts on that?
Yeah, I think it makes more sense to only error for explicit -mpcrel
-mcmodel={small,large}, linux64 sets cmodel as medium by default, I guess
that's why the current code doesn't check if the cmodel explicitly specified,
!medium implies it's changed explicitly. So if we move the checkings
to subtarget, it seems to have good locality (context)?
BR,
Kewen
