Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Michael Meissner]

On Fri, Nov 10, 2023 at 06:03:40PM -0600, Peter Bergner wrote:
> On 8/25/23 6:20 AM, Kewen.Lin wrote:
> > 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.
> 
> So my patch removes the flag from the default power10 flags, like
> you want.  However, it doesn't remove it from OTHER_POWER10_MASKS,
> since that is used to set ISA_3_1_MASKS_SERVER and I didn't want
> to change how rs6000_machine_from_flags() behaves, so instead, I
> just explicitly mask it off when defining the power10 default flags.

Historically the reason behind the two methods is they were done by different
people in parallel.  I had done the mask first in my pc-rel patches, but it
took a long time to integrate these into the compiler.  Bill Schmidt did the
functions as part of another change (rewriting built-ins maybe)

But prefixed and pc-rel cannot be added willy-nilly with -mcpu=power10 due to
conforming with other parts of the system (assembler, linker, ABIs, etc.).

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meiss...@linux.ibm.com

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner]

On 11/13/23 8:33 PM, Kewen.Lin wrote:
>> if (PCREL_SUPPORTED_BY_OS)
> 
>> +  else
>> +{
>> +  if (TARGET_PCREL
>> +  && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
>> +error ("use of %qs is invalid for this target", "-mpcrel");
>>rs6000_isa_flags &= ~OPTION_MASK_PCREL;
>>  }
> 
> now, I think it should be fine not to explicitly mask it off Power10
> default flags?  Then leave Power10 default flags unchanged seems more
> consistent.  The others look good to me!  Thanks again.

Ah, good catch, yes, I'll remove the clearing of the bit, since we know
it's either already clear (because that is the default value) or if it's
set, then that's the error condition we're catching here.  That also means
that we can remove the test for rs6000_isa_flags_explicit & OPTION_MASK_PCREL) 
!= 0
too, since at this point, TARGET_PCREL can only be true if it was explicitly
enabled with -mpcrel by the user.  Therefor, the code can now look like:

  if (PCREL_SUPPORTED_BY_OS)

  else if (TARGET_PCREL)
error ("use of %qs is invalid for this target", "-mpcrel");

I'll make that change, redo the bootstrap and regtesting and then
officially submit the patch.  Thanks!

Peter

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Kewen.Lin]

Hi Peter,

on 2023/11/11 07:51, Peter Bergner wrote:
> On 8/27/23 9:06 PM, Kewen.Lin wrote:
>> 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]
>> 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.
> 
> Ok, how about the patch below?  This removes OPTION_MASK_PCREL from the
> power10 flags, so instead of our options override code needing to disable
> PCREL on the systems that don't support it, we now enable it only on those
> systems that do support it.

Thanks for the patch!  I like the simplification by replacing function
rs6000_pcrel_p with TARGET_PCREL!  As the consideration on function 
rs6000_machine_from_flags and ISA_3_1_MASKS_SERVER you pointed out in
another mail, I agree it's not a good idea to remove OPTION_MASK_PCREL
from OTHER_POWER10_MASKS.  Since we have the code:

> if (PCREL_SUPPORTED_BY_OS)

> +  else
> +{
> +  if (TARGET_PCREL
> +   && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
> + error ("use of %qs is invalid for this target", "-mpcrel");
>rs6000_isa_flags &= ~OPTION_MASK_PCREL;
>  }

now, I think it should be fine not to explicitly mask it off Power10
default flags?  Then leave Power10 default flags unchanged seems more
consistent.  The others look good to me!  Thanks again.

BR,
Kewen

> 
> Jeevitha, can you test this patch to see whether it fixes the testsuite
> issue caused by your earlier patch that was approved, but not yet pushed?
> That was the use GPR2 for register allocation, correct?  Note, you'll need
> to update the patch to replace the rs6000_pcrel_p() usage with just
> TARGET_PCREL, since this patch removes rs6000_pcrel_p().
> 
> If testing is clean and everyone is OK with the patch, I'll officially
> submit it for review with git log entry, etc.
> 
> Peter
> 
> 
> gcc/
>   * config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Only test the ABI.
>   * config/rs6000/rs6000-cpus.def (RS6000_CPU): Remove OPTION_MASK_PCREL
>   from power10.
>   * config/rs6000/predicates.md: Use TARGET_PCREL.
>   * config/rs6000/rs6000-logue.cc (rs6000_decl_ok_for_sibcall): Likewise.
>   (rs6000_global_entry_point_prologue_needed_p): Likewise.
>   (rs6000_output_function_prologue): Likewise.
>   * config/rs6000/rs6000.md: Likewise.
>   * config/rs6000/rs6000.cc (rs6000_option_override_internal): Rework
>   the logic for enabling PCREL by default.
>   (rs6000_legitimize_tls_address): Use TARGET_PCREL.
>   (rs6000_call_template_1): Likewise.
>   (rs6000_indirect_call_template_1): Likewise.
>   (rs6000_longcall_ref): Likewise.
>   (rs6000_call_aix): Likewise.
>   (rs6000_sibcall_aix): Likewise.
>   (rs6000_pcrel_p): Remove.
>   * config/rs6000/rs6000-protos.h (rs6000_pcrel_p): Likewise.
> 
> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
> index 98b7255c95f..5b77bd7fd51 100644
> --- a/gcc/config/rs6000/linux64.h
> +++ b/gcc/config/rs6000/linux64.h
> @@ -563,8 +563,5 @@ extern int dot_symbols;
>  #define TARGET_FLOAT128_ENABLE_TYPE 1
>  
>  /* Enable using prefixed PC-relative addressing on POWER10 if the ABI
> -   supports it.  The ELF v2 ABI only supports PC-relative relocations for
> -   the medium code model.  */
> -#define PCREL_SUPPORTED_BY_OS(TARGET_POWER10 && TARGET_PREFIXED  
> \
> -  && ELFv2_ABI_CHECK \
> -  && TARGET_CMODEL == CMODEL_MEDIUM)
> +   supports it.  */
> +#define PCREL_SUPPORTED_BY_OS(ELFv2_ABI_CHECK)
> diff --git a/gcc/config/rs6000/rs6000-cpus.def 
> b/gcc/config/rs6000/rs6000-cpus.def
> index 4f350da378c..fe01a2312ae 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -256,7 +256,8 @@ RS6000_CPU ("power8", PROCESSOR_POWER8, MASK_POWERPC64 | 
> ISA_2_7_MASKS_SERVER
>   | OPTION_MASK_HTM)
>  RS6000_CPU ("power9", PROCESSOR_POWER9, MASK_POWERPC64 | ISA_3_0_MASKS_SERVER
>   | OPTION_MASK_HTM)
> -RS6000_CPU ("power10", PROCESSOR_POWER10, MASK_POWERPC64 | 
> ISA_3_1_MASKS_SERVER)
> +RS6000_CPU ("power10", PROCESSOR_POWER10, MASK_POWERPC64
> + | (ISA_3_1_MASKS_SERVER & ~OPTION_MASK_PCREL))
>  RS6000_CPU ("powerpc", PROCESSOR_POWERPC, 0)
>  RS6000_CPU ("powerpc64", PROCESSOR_POWERPC64, OPTION_MASK_PPC_GFXOPT
>   | MASK_POWERPC64)
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[jeevitha]

On 11/11/23 5:21 am, Peter Bergner wrote:

> Jeevitha, can you test this patch to see whether it fixes the testsuite
> issue caused by your earlier patch that was approved, but not yet pushed?
> That was the use GPR2 for register allocation, correct?  Note, you'll need
> to update the patch to replace the rs6000_pcrel_p() usage with just
> TARGET_PCREL, since this patch removes rs6000_pcrel_p().

Yeah Peter. This patch fixes the testsuite issue for GPR2 register allocation
patch [PR110320].

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner]

On 8/25/23 6:20 AM, Kewen.Lin wrote:
> 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.

So my patch removes the flag from the default power10 flags, like
you want.  However, it doesn't remove it from OTHER_POWER10_MASKS,
since that is used to set ISA_3_1_MASKS_SERVER and I didn't want
to change how rs6000_machine_from_flags() behaves, so instead, I
just explicitly mask it off when defining the power10 default flags.

Peter

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner]

On 8/27/23 9:06 PM, Kewen.Lin wrote:
> 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]
> 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.

Ok, how about the patch below?  This removes OPTION_MASK_PCREL from the
power10 flags, so instead of our options override code needing to disable
PCREL on the systems that don't support it, we now enable it only on those
systems that do support it.

Jeevitha, can you test this patch to see whether it fixes the testsuite
issue caused by your earlier patch that was approved, but not yet pushed?
That was the use GPR2 for register allocation, correct?  Note, you'll need
to update the patch to replace the rs6000_pcrel_p() usage with just
TARGET_PCREL, since this patch removes rs6000_pcrel_p().

If testing is clean and everyone is OK with the patch, I'll officially
submit it for review with git log entry, etc.

Peter


gcc/
* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Only test the ABI.
* config/rs6000/rs6000-cpus.def (RS6000_CPU): Remove OPTION_MASK_PCREL
from power10.
* config/rs6000/predicates.md: Use TARGET_PCREL.
* config/rs6000/rs6000-logue.cc (rs6000_decl_ok_for_sibcall): Likewise.
(rs6000_global_entry_point_prologue_needed_p): Likewise.
(rs6000_output_function_prologue): Likewise.
* config/rs6000/rs6000.md: Likewise.
* config/rs6000/rs6000.cc (rs6000_option_override_internal): Rework
the logic for enabling PCREL by default.
(rs6000_legitimize_tls_address): Use TARGET_PCREL.
(rs6000_call_template_1): Likewise.
(rs6000_indirect_call_template_1): Likewise.
(rs6000_longcall_ref): Likewise.
(rs6000_call_aix): Likewise.
(rs6000_sibcall_aix): Likewise.
(rs6000_pcrel_p): Remove.
* config/rs6000/rs6000-protos.h (rs6000_pcrel_p): Likewise.

diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
index 98b7255c95f..5b77bd7fd51 100644
--- a/gcc/config/rs6000/linux64.h
+++ b/gcc/config/rs6000/linux64.h
@@ -563,8 +563,5 @@ extern int dot_symbols;
 #define TARGET_FLOAT128_ENABLE_TYPE 1
 
 /* Enable using prefixed PC-relative addressing on POWER10 if the ABI
-   supports it.  The ELF v2 ABI only supports PC-relative relocations for
-   the medium code model.  */
-#define PCREL_SUPPORTED_BY_OS  (TARGET_POWER10 && TARGET_PREFIXED  \
-&& ELFv2_ABI_CHECK \
-&& TARGET_CMODEL == CMODEL_MEDIUM)
+   supports it.  */
+#define PCREL_SUPPORTED_BY_OS  (ELFv2_ABI_CHECK)
diff --git a/gcc/config/rs6000/rs6000-cpus.def 
b/gcc/config/rs6000/rs6000-cpus.def
index 4f350da378c..fe01a2312ae 100644
--- a/gcc/config/rs6000/rs6000-cpus.def
+++ b/gcc/config/rs6000/rs6000-cpus.def
@@ -256,7 +256,8 @@ RS6000_CPU ("power8", PROCESSOR_POWER8, MASK_POWERPC64 | 
ISA_2_7_MASKS_SERVER
| OPTION_MASK_HTM)
 RS6000_CPU ("power9", PROCESSOR_POWER9, MASK_POWERPC64 | ISA_3_0_MASKS_SERVER
| OPTION_MASK_HTM)
-RS6000_CPU ("power10", PROCESSOR_POWER10, MASK_POWERPC64 | 
ISA_3_1_MASKS_SERVER)
+RS6000_CPU ("power10", PROCESSOR_POWER10, MASK_POWERPC64
+   | (ISA_3_1_MASKS_SERVER & ~OPTION_MASK_PCREL))
 RS6000_CPU ("powerpc", PROCESSOR_POWERPC, 0)
 RS6000_CPU ("powerpc64", PROCESSOR_POWERPC64, OPTION_MASK_PPC_GFXOPT
| MASK_POWERPC64)
diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index ef7d3f214c4..0b76541fc0a 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1216,7 +1216,7 @@
 && SYMBOL_REF_DECL (op) != NULL
 && TREE_CODE (SYMBOL_REF_DECL (op)) == FUNCTION_DECL
 && (rs6000_fndecl_pcrel_p (SYMBOL_REF_DECL (op))
-!= rs6000_pcrel_p ()))")))
+!= TARGET_PCREL))")))
 
 ;; Return 1 if this operand is a valid input for a move insn.
 (define_predicate "input_operand"
diff --git a/gcc/config/rs6000/rs6000-logue.cc 
b/gcc/config/rs6000/rs6000-logue.cc
index 98846f781ec..9e08d9bb4d2 100644
--- a/gcc/config/rs6000/rs6000-logue.cc
+++ b/gcc/config/rs6000/rs6000-logue.cc
@@ -1106,7 +1106,7 @@ rs6000_decl_ok_for_sibcall (tree decl)
 r2 for its caller's TOC.  Such a function may make sibcalls to any
 function, whether local or external, without restriction based 

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Kewen.Lin via Gcc-patches]

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

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner via Gcc-patches]

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

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Kewen.Lin via Gcc-patches]

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

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner via Gcc-patches]

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:

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");

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");
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?

Peter

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Kewen.Lin via Gcc-patches]

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
> 
> 

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Peter Bergner via Gcc-patches]

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

Re: [PATCH] rs6000: Disable PCREL for unsupported targets [PR111045]

[Kewen.Lin via Gcc-patches]

Hi Jeevitha,

on 2023/8/21 18:32, jeevitha wrote:
> Hi All,
> 
> 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.

> 
> It is currently possible to incorrectly enable PCREL for targets that do not
> officially support it. Disable PCREL for targets that do not support it.
> 
> 2023-08-21  Jeevitha Palanisamy  
> 
> gcc/
>   PR target/111045
>   * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disable 
> PCREL
>   for unsupported targets.
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index efe9adc..4838f8c 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4232,6 +4232,9 @@ rs6000_option_override_internal (bool global_init_p)
>rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW;
>  }
>  
> +  if (!rs6000_pcrel_p())

Nit: Only do this check when TARGET_PCREL, one space before "()",
that is "...pcrel_p ()".

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;
}

==>

   else if (TARGET_PCREL && !rs6000_pcrel_p ())
  rs6000_isa_flags &= ~OPTION_MASK_PCREL;

Use DEFAULT_ABI != ABI_ELFv2 seems more straightforward, but I guessed the
reason why you used rs6000_pcrel_p is to avoid scattered conditions, it's
fine to me.  Besides, I think we want one error for explicit -mpcrel on
unsupported targets just similar to the above cmodel?

BR,
Kewen

> +rs6000_isa_flags &= ~OPTION_MASK_PCREL;
> +
>/* Enable -mprefixed by default on power10 systems.  */
>if (TARGET_POWER10 && (rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED) 
> == 0)
>  rs6000_isa_flags |= OPTION_MASK_PREFIXED;
>