on 2023/8/26 06:04, Peter Bergner wrote: > 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.
Thanks for clarifying this! Yes, I know the pcrel support requires TARGET_PREFIXED (and its required TARGET_POWER10), but IMHO these TARGET_PREFIXED and TARGET_POWER10 are common for any subtargets which support or will support pcrel, they can be checked in common code, instead of being a part of PCREL_SUPPORTED_BY_OS. We already has the code to check pcrel's need on prefixed and prefixed's need on Power10, we can just move these checkings after PCREL_SUPPORTED_BY_OS check. Assuming we only have ELFv2_ABI_CHECK in PCREL_SUPPORTED_BY_OS, we can have either TARGET_PCREL or !TARGET_PCREL after the checking. For the latter, it's fine and don't need any checks. For the former, if it's implicit, for !TARGET_PREFIXED we will clean it silently; while if it's explicit, for !TARGET_PREFIXED we will emit an error. TARGET_PREFIXED checking has considered Power10, so it's also concerned accordingly. > > > [snip ...] > > >> 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. Yeah, looking forward to their opinions. IMHO, with the current proposed change, pcrel doesn't look like a pure Power10 hardware feature, it also quite relies on ABIs, that's why I thought it seems good not to turn it on by default for Power10. BR, Kewen
