Re: [PATCH] rs6000: Only enable PCREL on supported ABIs [PR111045]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi,

on 2023/11/15 11:01, Peter Bergner wrote:
> PCREL data accesses are only officially supported on ELFv2.  We currently
> incorrectly enable PCREL on all Power10 compiles in which prefix instructions
> are also enabled.  Rework the option override code so we only enable PCREL
> for those ABIs that actually support it.
> 
> Jeevitha has confirmed this patch fixes the testsuite fallout seen with her
> PR110320 patch.
> 
> This has been bootstrapped and regtested with no regressions on the following
> builds: powerpc64le-linux, powerpc64le-linux --with-cpu=power10 and
> powerpc64-linux - testsuite run in both 32-bit and 64-bit modes.
> Ok for trunk?

OK for trunk and backporting, but wait for two days or so in case Segher and
David have some comments, thanks!

BR,
Kewen

> 
> Ok for the release branches after some burn-in on trunk?
> 
> Peter
> 
> 
> gcc/
> 	PR target/111045
> 	* 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.
> 
> gcc/testsuite/
> 	PR target/111045
> 	* gcc.target/powerpc/pr111045.c: New test.
> 	* gcc.target/powerpc/float128-constant.c: Add instruction counts for
> 	non-pcrel compiles.
> 
> 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 on
>  	 TOC-save/restore rules.  */
> -      if (rs6000_pcrel_p ())
> +      if (TARGET_PCREL)
>  	return true;
>  
>        /* Otherwise, under the AIX or ELFv2 ABIs we can't allow sibcalls
> @@ -2583,7 +2583,7 @@ rs6000_global_entry_point_prologue_needed_p (void)
>      return false;
>  
>    /* PC-relative functions never generate a global entry point prologue.  */
> -  if (rs6000_pcrel_p ())
> +  if (TARGET_PCREL)
>      return false;
>  
>    /* Ensure we have a global entry point for thunks.   ??? We could
> @@ -4045,7 +4045,7 @@ rs6000_output_function_prologue (FILE *file)
>  					       patch_area_entry == 0);
>      }
>  
> -  else if (rs6000_pcrel_p ())
> +  else if (TARGET_PCREL)
>      {
>        const char *name = XSTR (XEXP (DECL_RTL (current_function_decl), 0), 0);
>        /* All functions compiled to use PC-relative addressing will
> diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h
> index f70118ea40f..8ffec295d09 100644
> --- a/gcc/config/rs6000/rs6000-protos.h
> +++ b/gcc/config/rs6000/rs6000-protos.h
> @@ -158,7 +158,6 @@ extern rtx rs6000_allocate_stack_temp (machine_mode, bool, bool);
>  extern align_flags rs6000_loop_align (rtx);
>  extern void rs6000_split_logical (rtx [], enum rtx_code, bool, bool, bool);
>  extern bool rs6000_function_pcrel_p (struct function *);
> -extern bool rs6000_pcrel_p (void);
>  extern bool rs6000_fndecl_pcrel_p (const_tree);
>  extern void rs6000_output_addr_vec_elt (FILE *, int);
>  
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 5f56c3ed85b..0c208cc2c0a 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4378,19 +4378,22 @@ rs6000_option_override_internal (bool global_init_p)
>    /* 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 (PCREL_SUPPORTED_BY_OS)
>      {
> -      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
> +      /* PCREL on ELFv2 currently requires -mcmodel=medium, but we can't check
> +	 TARGET_CMODEL until after the subtarget override options are done.  */
> +      if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
>  	error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
>  
> -      rs6000_isa_flags &= ~OPTION_MASK_PCREL;
> +      if (!TARGET_PCREL
> +	  && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0
> +	  && TARGET_POWER10
> +	  && TARGET_PREFIXED
> +	  && TARGET_CMODEL == CMODEL_MEDIUM)
> +	rs6000_isa_flags |= OPTION_MASK_PCREL;
>      }
> +  else if (TARGET_PCREL)
> +    error ("use of %qs is invalid for this target", "-mpcrel");
>  
>    /* Enable -mmma by default on power10 systems.  */
>    if (TARGET_POWER10 && (rs6000_isa_flags_explicit & OPTION_MASK_MMA) == 0)
> @@ -9645,7 +9648,7 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model)
>  
>    dest = gen_reg_rtx (Pmode);
>    if (model == TLS_MODEL_LOCAL_EXEC
> -      && (rs6000_tls_size == 16 || rs6000_pcrel_p ()))
> +      && (rs6000_tls_size == 16 || TARGET_PCREL))
>      {
>        rtx tlsreg;
>  
> @@ -9692,7 +9695,7 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model)
>  	 them in the .got section.  So use a pointer to the .got section,
>  	 not one to secondary TOC sections used by 64-bit -mminimal-toc,
>  	 or to secondary GOT sections used by 32-bit -fPIC.  */
> -      if (rs6000_pcrel_p ())
> +      if (TARGET_PCREL)
>  	got = const0_rtx;
>        else if (TARGET_64BIT)
>  	got = gen_rtx_REG (Pmode, 2);
> @@ -9757,7 +9760,7 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model)
>  	  rtx uns = gen_rtx_UNSPEC (Pmode, vec, UNSPEC_TLS_GET_ADDR);
>  	  set_unique_reg_note (get_last_insn (), REG_EQUAL, uns);
>  
> -	  if (rs6000_tls_size == 16 || rs6000_pcrel_p ())
> +	  if (rs6000_tls_size == 16 || TARGET_PCREL)
>  	    {
>  	      if (TARGET_64BIT)
>  		insn = gen_tls_dtprel_64 (dest, tmp1, addr);
> @@ -9798,7 +9801,7 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model)
>  	  else
>  	    insn = gen_tls_got_tprel_32 (tmp2, got, addr);
>  	  emit_insn (insn);
> -	  if (rs6000_pcrel_p ())
> +	  if (TARGET_PCREL)
>  	    {
>  	      if (TARGET_64BIT)
>  		insn = gen_tls_tls_pcrel_64 (dest, tmp2, addr);
> @@ -14866,7 +14869,7 @@ rs6000_call_template_1 (rtx *operands, unsigned int funop, bool sibcall)
>  	    ? "+32768" : ""));
>  
>    static char str[32];  /* 1 spare */
> -  if (rs6000_pcrel_p ())
> +  if (TARGET_PCREL)
>      sprintf (str, "b%s %s@notoc%s", sibcall ? "" : "l", z, arg);
>    else if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
>      sprintf (str, "b%s %s%s%s", sibcall ? "" : "l", z, arg,
> @@ -15006,7 +15009,7 @@ rs6000_indirect_call_template_1 (rtx *operands, unsigned int funop,
>  		     rel64);
>  	}
>  
> -      const char *notoc = rs6000_pcrel_p () ? "_NOTOC" : "";
> +      const char *notoc = TARGET_PCREL ? "_NOTOC" : "";
>        const char *addend = (DEFAULT_ABI == ABI_V4 && TARGET_SECURE_PLT
>  			    && flag_pic == 2 ? "+32768" : "");
>        if (!speculate)
> @@ -15023,7 +15026,7 @@ rs6000_indirect_call_template_1 (rtx *operands, unsigned int funop,
>    else if (!speculate)
>      s += sprintf (s, "crset 2\n\t");
>  
> -  if (rs6000_pcrel_p ())
> +  if (TARGET_PCREL)
>      {
>        if (speculate)
>  	sprintf (s, "b%%T%ul", funop);
> @@ -20685,7 +20688,7 @@ rs6000_longcall_ref (rtx call_ref, rtx arg)
>      {
>        rtx base = const0_rtx;
>        int regno = 12;
> -      if (rs6000_pcrel_p ())
> +      if (TARGET_PCREL)
>  	{
>  	  rtx reg = gen_rtx_REG (Pmode, regno);
>  	  rtx u = gen_rtx_UNSPEC_VOLATILE (Pmode,
> @@ -25934,7 +25937,7 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>    if (!SYMBOL_REF_P (func)
>        || (DEFAULT_ABI == ABI_AIX && !SYMBOL_REF_FUNCTION_P (func)))
>      {
> -      if (!rs6000_pcrel_p ())
> +      if (!TARGET_PCREL)
>  	{
>  	  /* Save the TOC into its reserved slot before the call,
>  	     and prepare to restore it after the call.  */
> @@ -26040,7 +26043,7 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>    else
>      {
>        /* No TOC register needed for calls from PC-relative callers.  */
> -      if (!rs6000_pcrel_p ())
> +      if (!TARGET_PCREL)
>  	/* Direct calls use the TOC: for local calls, the callee will
>  	   assume the TOC register is set; for non-local calls, the
>  	   PLT stub needs the TOC register.  */
> @@ -26089,7 +26092,7 @@ rs6000_sibcall_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>      {
>        /* PCREL can do a sibling call to a longcall function
>  	 because we don't need to restore the TOC register.  */
> -      gcc_assert (rs6000_pcrel_p ());
> +      gcc_assert (TARGET_PCREL);
>        func_desc = rs6000_longcall_ref (func_desc, tlsarg);
>      }
>    else
> @@ -26116,7 +26119,7 @@ rs6000_sibcall_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>    insn = emit_call_insn (insn);
>  
>    /* Note use of the TOC register.  */
> -  if (!rs6000_pcrel_p ())
> +  if (!TARGET_PCREL)
>      use_reg (&CALL_INSN_FUNCTION_USAGE (insn),
>  	     gen_rtx_REG (Pmode, TOC_REGNUM));
>  
> @@ -26416,16 +26419,6 @@ rs6000_function_pcrel_p (struct function *fn)
>    return rs6000_fndecl_pcrel_p (fn->decl);
>  }
>  
> -/* Return whether we should generate PC-relative code for the current
> -   function.  */
> -bool
> -rs6000_pcrel_p ()
> -{
> -  return (DEFAULT_ABI == ABI_ELFv2
> -	  && (rs6000_isa_flags & OPTION_MASK_PCREL) != 0
> -	  && TARGET_CMODEL == CMODEL_MEDIUM);
> -}
> -
>  \f
>  /* Given an address (ADDR), a mode (MODE), and what the format of the
>     non-prefixed address (NON_PREFIXED_FORMAT) is, return the instruction format
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 2a1b5ecfaee..97de6940e07 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -11333,7 +11333,7 @@
>  			    (match_operand:P 3 "" "")]
>  			   UNSPECV_PLT_PCREL))]
>    "HAVE_AS_PLTSEQ && TARGET_ELF
> -   && rs6000_pcrel_p ()"
> +   && TARGET_PCREL"
>  {
>    return rs6000_pltseq_template (operands, RS6000_PLTSEQ_PLT_PCREL34);
>  }
> @@ -11418,7 +11418,7 @@
>    else if (INTVAL (operands[2]) & CALL_V4_CLEAR_FP_ARGS)
>      output_asm_insn ("creqv 6,6,6", operands);
>  
> -  if (rs6000_pcrel_p ())
> +  if (TARGET_PCREL)
>      return "bl %z0@notoc";
>    return (DEFAULT_ABI == ABI_V4 && flag_pic) ? "bl %z0@local" : "bl %z0";
>  }
> @@ -11439,7 +11439,7 @@
>    else if (INTVAL (operands[3]) & CALL_V4_CLEAR_FP_ARGS)
>      output_asm_insn ("creqv 6,6,6", operands);
>  
> -  if (rs6000_pcrel_p ())
> +  if (TARGET_PCREL)
>      return "bl %z1@notoc";
>    return (DEFAULT_ABI == ABI_V4 && flag_pic) ? "bl %z1@local" : "bl %z1";
>  }
> @@ -11614,7 +11614,7 @@
>  }
>    [(set_attr "type" "branch")
>     (set (attr "length")
> -	(if_then_else (match_test "rs6000_pcrel_p ()")
> +	(if_then_else (match_test "TARGET_PCREL")
>  	  (const_int 4)
>  	  (const_int 8)))])
>  
> @@ -11631,7 +11631,7 @@
>  }
>    [(set_attr "type" "branch")
>     (set (attr "length")
> -	(if_then_else (match_test "rs6000_pcrel_p ()")
> +	(if_then_else (match_test "TARGET_PCREL")
>  	    (const_int 4)
>  	    (const_int 8)))])
>  
> @@ -11705,7 +11705,7 @@
>  	 (match_operand 1))
>     (use (match_operand:SI 2 "immediate_operand" "n,n,n"))
>     (clobber (reg:P LR_REGNO))]
> -  "rs6000_pcrel_p ()"
> +  "TARGET_PCREL"
>  {
>    return rs6000_indirect_call_template (operands, 0);
>  }
> @@ -11742,7 +11742,7 @@
>  	      (match_operand:P 2 "unspec_tls" "")))
>     (use (match_operand:SI 3 "immediate_operand" "n,n,n"))
>     (clobber (reg:P LR_REGNO))]
> -  "rs6000_pcrel_p ()"
> +  "TARGET_PCREL"
>  {
>    return rs6000_indirect_call_template (operands, 1);
>  }
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr111045.c b/gcc/testsuite/gcc.target/powerpc/pr111045.c
> new file mode 100644
> index 00000000000..6fd2773eb35
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr111045.c
> @@ -0,0 +1,15 @@
> +/* PR target/111045 */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +
> +/* Verify we do not generate a PCREL access on ABIs that doesn't support it.  */
> +
> +static unsigned short s;
> +
> +unsigned short *
> +foo (void)
> +{
> +  return &s;
> +}
> +
> +/* { dg-final { scan-assembler-not {\m@pcrel\M} { target { ! powerpc_pcrel } } } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/float128-constant.c b/gcc/testsuite/gcc.target/powerpc/float128-constant.c
> index e3286a786a5..4728f4891da 100644
> --- a/gcc/testsuite/gcc.target/powerpc/float128-constant.c
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-constant.c
> @@ -155,6 +155,7 @@ return_longlong_neg_0 (void)
>  }
>  
>  /* { dg-final { scan-assembler-times {\mlxvkq\M}    19 } } */
> -/* { dg-final { scan-assembler-times {\mplxv\M}      3 } } */
> +/* { dg-final { scan-assembler-times {\mplxv\M}      3 { target { powerpc_pcrel } } } } */
> +/* { dg-final { scan-assembler-times {\mlxv\M}       3 { target { ! powerpc_pcrel } } } } */
>  /* { dg-final { scan-assembler-times {\mxxspltib\M}  1 } } */
>