diff mbox series

, PowerPC, Patch #6, Create pc-relative addressing insns

Message ID 20190709223000.GA18842@ibm-toto.the-meissners.org
State New
Headers show
Series , PowerPC, Patch #6, Create pc-relative addressing insns | expand

Commit Message

Michael Meissner July 9, 2019, 10:30 p.m. UTC 
This patch updates the basic support for pc-relative addressing that will be
added in a future machine.

It was originally proposed as patch #4:
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01866.html

Segher suggested that I split out moving create_TOC_reference back to rs6000.c
as a separate patch, which I did in this patch that has been committed:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00719.html

In working on the other suggestions from Segher about the second argument to
create_TOC_reference (which is where to put the HIGH value after register
allocation), I came to the conclusion it wasn't worth it to try and combine TOC
and pc-relative addressing.  There are two places in rs6000_emit_move that deal
with the creation of TOC/pc-relative addressing.

There is also one place in rs6000.md that created TOC addressing that needed to
support pc-relative addressing also.  It does the compare of two __ibm128
floating point values when the -mcompat-xl switch is used, and the comparison
needs to compare the values against 0 and +infinity, and it needs to load up
the two constants.

Instead of trying to combine the two addressing forms, I added separate support
for pc-relative addressing, and added extra checks in supporting TOC
addressing.

The one place where I combined TOC and pc-relative addressing is using a common
function to set the memory alias set group.  Originally it was called
get_TOC_alias_set, and I renamed it to get_data_alias_set and I changed all of
the callers.

There will be several other patches before all of the pc-relative support is
available and turned on by default.

I have bootstraped the compiler on a little endian power8 system and there were
no regressions in the test suite.  Can I check it into the trunk?

2019-07-09  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-protos.h (get_data_alias_set): Rename
	get_TOC_alias_set for use with both TOC & pc-relative addressing.
	* config/rs6000/rs6000.c (create_TOC_reference): Add gcc_assert.
	(rs6000_legitimize_address): Add check for not pc-relative in TOC
	support.
	(rs6000_legitimize_tls_address_aix): Call get_data_alias_set
	instead of get_TOC_alias_set.
	(use_toc_relative_ref): Add check for SYMBOL_REF and not
	pc-relative to simplify callers.  Make tests easier to
	understand.
	(use_pc_relative_ref): New helper function.
	(rs6000_emit_move): Add basic support for pc-relative addressing.
	Call get_data_alias_set instead of get_TOC_alias_set.
	(data_alias_set): Rename static variable.
	(get_data_alias_set): Rename get_TOC_alias_set since it is now
	used for both TOC and pc-relative addressing.
	* config/rs6000/rs6000.md (cmp<mode>_internal2): Add support for
	pc-relative addressing.  Call get_data_alias_set instead of
	get_TOC_alias_set.

Comments

Segher Boessenkool July 10, 2019, 5:11 p.m. UTC | #1 
Hi Mike,

On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
>  static bool
>  use_toc_relative_ref (rtx sym, machine_mode mode)
>  {
> -  return ((constant_pool_expr_p (sym)
> -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> -					       get_pool_mode (sym)))
> -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> -	      && SYMBOL_REF_LOCAL_P (sym)
> -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> +    return false;

Why the SYMBOL_REF test?  The original didn't have any.  But its comment
says
  /* Return true iff the given SYMBOL_REF refers to a constant pool entry
     that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
     can be addressed relative to the toc pointer.  */
so perhaps you want an assert instead.

> +
> +  if (constant_pool_expr_p (sym)
> +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> +					  get_pool_mode (sym)))
> +    return true;
> +
> +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);

Please don't put disparate things on one line, it was fine before.

> +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
> +   have put in the pc-relative constant area, or for cmodel=medium, if the
> +   SYMBOL_REF can be addressed via pc-relative instructions.  */
> +
> +static bool
> +use_pc_relative_ref (rtx sym)
> +{
> +  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> +    return false;

Same here, assert please.  Or nothing, it will ICE not much later anyway.
But don't silently return if something violates the call contract.

> -static GTY(()) alias_set_type set = -1;
> +/* Return the alias set for constants stored in either the TOC or via
> +   pc-relative addressing.  */
> +static GTY(()) alias_set_type data_alias_set = -1;

Please just make a separate alias set for pcrel.  The new name isn't as
bad as just "set" :-), but "data"?  That's not great either.  Conflating
toc and pcrel isn't a good thing.

(Variables don't "return" anything btw).

>  
>  alias_set_type
> -get_TOC_alias_set (void)
> +get_data_alias_set (void)

This name is much worse than the old one.  Just make separate functions
for TOC and for pcrel?  Or is there any reason you want to share them?


Segher
Michael Meissner July 10, 2019, 5:56 p.m. UTC | #2 
On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> > @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
> >  static bool
> >  use_toc_relative_ref (rtx sym, machine_mode mode)
> >  {
> > -  return ((constant_pool_expr_p (sym)
> > -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > -					       get_pool_mode (sym)))
> > -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> > -	      && SYMBOL_REF_LOCAL_P (sym)
> > -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> > +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> > +    return false;
> 
> Why the SYMBOL_REF test?  The original didn't have any.  But its comment
> says
>   /* Return true iff the given SYMBOL_REF refers to a constant pool entry
>      that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
>      can be addressed relative to the toc pointer.  */
> so perhaps you want an assert instead.

The only two callers had a test for SYMBOL_REF_P.  By moving it into the
function, it made the call to the function one line instead of two.  But I can
go back to the previous method.

> > +
> > +  if (constant_pool_expr_p (sym)
> > +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > +					  get_pool_mode (sym)))
> > +    return true;
> > +
> > +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> > +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
> 
> Please don't put disparate things on one line, it was fine before.

I'm not sure what you mean, I was just trying to break up a long if statement.

> > +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
> > +   have put in the pc-relative constant area, or for cmodel=medium, if the
> > +   SYMBOL_REF can be addressed via pc-relative instructions.  */
> > +
> > +static bool
> > +use_pc_relative_ref (rtx sym)
> > +{
> > +  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> > +    return false;
> 
> Same here, assert please.  Or nothing, it will ICE not much later anyway.
> But don't silently return if something violates the call contract.

Again, this was meant to simplify the call.

> > -static GTY(()) alias_set_type set = -1;
> > +/* Return the alias set for constants stored in either the TOC or via
> > +   pc-relative addressing.  */
> > +static GTY(()) alias_set_type data_alias_set = -1;
> 
> Please just make a separate alias set for pcrel.  The new name isn't as
> bad as just "set" :-), but "data"?  That's not great either.  Conflating
> toc and pcrel isn't a good thing.
> 
> (Variables don't "return" anything btw).
> 
> >  
> >  alias_set_type
> > -get_TOC_alias_set (void)
> > +get_data_alias_set (void)
> 
> This name is much worse than the old one.  Just make separate functions
> for TOC and for pcrel?  Or is there any reason you want to share them?

Well in theory, an object file that contains some functions wtih pc-relative
and some with TOC (due to #pragma target or attribute), using the same data set
would flag that they are related, but I imagine in practice the two uses don't
mix.  It was more of a hangover from trying to have one function to create both
addressing forms.
Segher Boessenkool July 10, 2019, 6:37 p.m. UTC | #3 
On Wed, Jul 10, 2019 at 01:56:07PM -0400, Michael Meissner wrote:
> On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> > On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> > > @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
> > >  static bool
> > >  use_toc_relative_ref (rtx sym, machine_mode mode)
> > >  {
> > > -  return ((constant_pool_expr_p (sym)
> > > -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > > -					       get_pool_mode (sym)))
> > > -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> > > -	      && SYMBOL_REF_LOCAL_P (sym)
> > > -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> > > +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> > > +    return false;
> > 
> > Why the SYMBOL_REF test?  The original didn't have any.  But its comment
> > says
> >   /* Return true iff the given SYMBOL_REF refers to a constant pool entry
> >      that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
> >      can be addressed relative to the toc pointer.  */
> > so perhaps you want an assert instead.
> 
> The only two callers had a test for SYMBOL_REF_P.  By moving it into the
> function, it made the call to the function one line instead of two.  But I can
> go back to the previous method.

It makes more sense with the param as a symbol always (the function
comment says it is, too, and the "sym" name suggests it is).  So yes,
please go back to that.  Reducing number of lines of code isn't a goal;
reducing complexity is, reducing astonishment.

> > > +
> > > +  if (constant_pool_expr_p (sym)
> > > +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > > +					  get_pool_mode (sym)))
> > > +    return true;
> > > +
> > > +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> > > +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
> > 
> > Please don't put disparate things on one line, it was fine before.
> 
> I'm not sure what you mean, I was just trying to break up a long if statement.

"TARGET_CMODEL == CMODEL_MEDIUM" and "SYMBOL_REF_LOCAL_P (sym)" are quite
different things.

> > > -static GTY(()) alias_set_type set = -1;
> > > +/* Return the alias set for constants stored in either the TOC or via
> > > +   pc-relative addressing.  */
> > > +static GTY(()) alias_set_type data_alias_set = -1;
> > 
> > Please just make a separate alias set for pcrel.  The new name isn't as
> > bad as just "set" :-), but "data"?  That's not great either.  Conflating
> > toc and pcrel isn't a good thing.
> > 
> > (Variables don't "return" anything btw).
> > 
> > >  alias_set_type
> > > -get_TOC_alias_set (void)
> > > +get_data_alias_set (void)
> > 
> > This name is much worse than the old one.  Just make separate functions
> > for TOC and for pcrel?  Or is there any reason you want to share them?
> 
> Well in theory, an object file that contains some functions wtih pc-relative
> and some with TOC (due to #pragma target or attribute), using the same data set
> would flag that they are related, but I imagine in practice the two uses don't
> mix.  It was more of a hangover from trying to have one function to create both
> addressing forms.

I think it would make things quite a bit simpler if you split them up.
Could you please try that?


Segher
diff mbox series

Patch

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273255)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -189,7 +189,7 @@  extern void rs6000_gen_section_name (cha
 extern void output_function_profiler (FILE *, int);
 extern void output_profile_hook  (int);
 extern int rs6000_trampoline_size (void);
-extern alias_set_type get_TOC_alias_set (void);
+extern alias_set_type get_data_alias_set (void);
 extern void rs6000_emit_prologue (void);
 extern void rs6000_emit_load_toc_table (int);
 extern unsigned int rs6000_dbx_register_number (unsigned int, unsigned int);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273310)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -7740,6 +7740,8 @@  create_TOC_reference (rtx symbol, rtx la
 {
   rtx tocrel, tocreg, hi;
 
+  gcc_assert (TARGET_TOC && !TARGET_PCREL);
+
   if (TARGET_DEBUG_ADDR)
     {
       if (SYMBOL_REF_P (symbol))
@@ -8137,7 +8139,7 @@  rs6000_legitimize_address (rtx x, rtx ol
 	emit_insn (gen_macho_high (reg, x));
       return gen_rtx_LO_SUM (Pmode, reg, x);
     }
-  else if (TARGET_TOC
+  else if (TARGET_TOC && !TARGET_PCREL
 	   && SYMBOL_REF_P (x)
 	   && constant_pool_expr_p (x)
 	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (x), Pmode))
@@ -8441,7 +8443,7 @@  rs6000_legitimize_tls_address_aix (rtx a
     {
       tocref = create_TOC_reference (XEXP (sym, 0), NULL_RTX);
       mem = gen_const_mem (Pmode, tocref);
-      set_mem_alias_set (mem, get_TOC_alias_set ());
+      set_mem_alias_set (mem, get_data_alias_set ());
     }
   else
     return sym;
@@ -8459,7 +8461,7 @@  rs6000_legitimize_tls_address_aix (rtx a
       SYMBOL_REF_FLAGS (modaddr) |= SYMBOL_FLAG_LOCAL;
       tocref = create_TOC_reference (modaddr, NULL_RTX);
       rtx modmem = gen_const_mem (Pmode, tocref);
-      set_mem_alias_set (modmem, get_TOC_alias_set ());
+      set_mem_alias_set (modmem, get_data_alias_set ());
       
       rtx modreg = gen_reg_rtx (Pmode);
       emit_insn (gen_rtx_SET (modreg, modmem));
@@ -8760,12 +8762,34 @@  rs6000_cannot_force_const_mem (machine_m
 static bool
 use_toc_relative_ref (rtx sym, machine_mode mode)
 {
-  return ((constant_pool_expr_p (sym)
-	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
-					       get_pool_mode (sym)))
-	  || (TARGET_CMODEL == CMODEL_MEDIUM
-	      && SYMBOL_REF_LOCAL_P (sym)
-	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
+  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
+    return false;
+
+  if (constant_pool_expr_p (sym)
+      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
+					  get_pool_mode (sym)))
+    return true;
+
+  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
+	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
+}
+
+/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
+   have put in the pc-relative constant area, or for cmodel=medium, if the
+   SYMBOL_REF can be addressed via pc-relative instructions.  */
+
+static bool
+use_pc_relative_ref (rtx sym)
+{
+  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
+    return false;
+
+  if (constant_pool_expr_p (sym)
+      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
+					  get_pool_mode (sym)))
+    return true;
+
+  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym));
 }
 
 /* TARGET_LEGITIMATE_ADDRESS_P recognizes an RTL expression
@@ -9865,10 +9889,14 @@  rs6000_emit_move (rtx dest, rtx source,
       /* If this is a SYMBOL_REF that refers to a constant pool entry,
 	 and we have put it in the TOC, we just need to make a TOC-relative
 	 reference to it.  */
-      if (TARGET_TOC
-	  && SYMBOL_REF_P (operands[1])
-	  && use_toc_relative_ref (operands[1], mode))
+      if (use_toc_relative_ref (operands[1], mode))
 	operands[1] = create_TOC_reference (operands[1], operands[0]);
+
+      /* If this is a pc-relative reference, we don't have to do anything
+	 fancy.  */
+      else if (use_pc_relative_ref (operands[1]))
+	;
+
       else if (mode == Pmode
 	       && CONSTANT_P (operands[1])
 	       && GET_CODE (operands[1]) != HIGH
@@ -9919,14 +9947,18 @@  rs6000_emit_move (rtx dest, rtx source,
 
 	  operands[1] = force_const_mem (mode, operands[1]);
 
-	  if (TARGET_TOC
-	      && SYMBOL_REF_P (XEXP (operands[1], 0))
-	      && use_toc_relative_ref (XEXP (operands[1], 0), mode))
+	  rtx addr = XEXP (operands[1], 0);
+	  if (use_toc_relative_ref (addr, mode))
 	    {
-	      rtx tocref = create_TOC_reference (XEXP (operands[1], 0),
-						 operands[0]);
+	      rtx tocref = create_TOC_reference (addr, operands[0]);
 	      operands[1] = gen_const_mem (mode, tocref);
-	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
+	      set_mem_alias_set (operands[1], get_data_alias_set ());
+	    }
+
+	  else if (use_pc_relative_ref (addr))
+	    {
+	      operands[1] = gen_const_mem (mode, addr);
+	      set_mem_alias_set (operands[1], get_data_alias_set ());
 	    }
 	}
       break;
@@ -23732,14 +23764,16 @@  rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+/* Return the alias set for constants stored in either the TOC or via
+   pc-relative addressing.  */
+static GTY(()) alias_set_type data_alias_set = -1;
 
 alias_set_type
-get_TOC_alias_set (void)
+get_data_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (data_alias_set == -1)
+    data_alias_set = new_alias_set ();
+  return data_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 273255)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11606,15 +11606,23 @@  (define_insn_and_split "*cmp<mode>_inter
   operands[15] = force_const_mem (DFmode,
 				  const_double_from_real_value (dconst0,
 								DFmode));
-  if (TARGET_TOC)
+  if (TARGET_PCREL)
+    {
+      operands[14] = gen_const_mem (DFmode, XEXP (operands[14], 0));
+      operands[15] = gen_const_mem (DFmode, XEXP (operands[15], 0));
+      set_mem_alias_set (operands[14], get_data_alias_set ());
+      set_mem_alias_set (operands[15], get_data_alias_set ());
+    }
+
+  else if (TARGET_TOC)
     {
       rtx tocref;
       tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]);
       operands[14] = gen_const_mem (DFmode, tocref);
       tocref = create_TOC_reference (XEXP (operands[15], 0), operands[11]);
       operands[15] = gen_const_mem (DFmode, tocref);
-      set_mem_alias_set (operands[14], get_TOC_alias_set ());
-      set_mem_alias_set (operands[15], get_TOC_alias_set ());
+      set_mem_alias_set (operands[14], get_data_alias_set ());
+      set_mem_alias_set (operands[15], get_data_alias_set ());
     }
 })