On 8/25/23 6:20 AM, Kewen.Lin wrote: > 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 pcrel instructions are 64-bit/prefix instructions, so I think for PCREL_SUPPORTED_BY_OS, we want to keep the TARGET_POWER10 and the TARGET_PREFIXED checks. Then we should have the checks for the OS that can support pcrel, in this case, only ELFv2_ABI_CHECK for now. I think we should remove the TARGET_CMODEL check, since that isn't strictly required, it's a current code requirement for ELFv2, but could change in the future. In fact, Mike has talked about adding pcrel support for ELFv2 and -mcmodel=large, so I think that should move moved out of PCREL_SUPPORTED_BY_OS and into the option override checks. > 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? As I said above, we should also keep TARGET_PREFIX, since that is required for the pcrel instructions, which are 64-bit/prefix insns. I think the MCMODEL check should be removed from PCREL_SUPPORTED_BY_OS and moved to the option override checks, since there's not technical reason the different MCMODEL options cannot be made to work with pcrel. > 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? Yeah, I think that would probably help simplify the tests, since they're semi-target specific already. Of course, we there is no SUB*TARGET_OVERRIDE_OPTIONS and the other OSes, so those would have to be added. > 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. I'm on the fence about this one and would like to hear from Segher and Mike on what they think. In some respect, pcrel is a Power10 hardware feature, so that would seem to make sense to set the flag, but yeah, we only have one OS that currently supports it, so maybe not setting it makes sense? Like I said, I think I need Segher and Mike to chime in with their thoughts. Peter
