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

Message ID 20190710211748.GA18544@ibm-toto.the-meissners.org
State New
Headers show
Series
  • , PowerPC, Patch #6, revision 2, Create pc-relative addressing insns
Related show

Commit Message

Michael Meissner July 10, 2019, 9:17 p.m.
Here is the revision of patch #6.  Changes from the previous version of the
patch:

1) I provided separate get_TOC_alias_set and get_pc_relative_alias_set
functions, and changed all of the places that had been changed to call
get_data_alias_set to revert to calling get_TOC_alias_set.

2) I added an assert in create_TOC_reference to guard against being called with
pc-relative addressing.

3) In the other places that check for TOC handling, I added tests to skip the
TOC handling if we are generating pc-relative code.

4) I added code in rs6000_emit_move to handle the local SYMBOL_REFs when
pc-relative moves are handled, as well as the constants that were created.

5) I simplified some of the code that set up prefixed addressing since we
already created a valid address when the constant was forced to memory.  I
realized that I didn't have to check whether the SYMBOL_REF was within the
constant pool with pc-relative addressing, I just had to check if it was a
local label and medium code model.

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

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

	* config/rs6000/rs6000-protos.h (get_pc_relative_alias_set): New
	declaration.
	* config/rs6000/rs6000.c (create_TOC_reference): Add gcc_assert.
	(toc_relative_expr_p): Add pc-relative check.
	(rs6000_legitimize_address): Add pc-relative check in TOC code.
	(rs6000_emit_move): Add support for loading up the addresses and
	constants with pc-relative addressing.
	(TOC_alias_set): Rename static variable from 'set'.
	(get_TOC_alias_set): Use TOC_alias_set.
	(pc_relative_alias_set): New static variable.
	(get_pc_relative_alias_set): New function.
	* config/rs6000/rs6000.md (cmp<mode>_internal2): Add support for
	pc-relative addressing.


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Comments

Segher Boessenkool July 11, 2019, 7:52 p.m. | #1
On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:
> +extern alias_set_type get_pc_relative_alias_set (void);


I'd just call it "pcrel", not "pc_relative", just like everywhere else?

> @@ -7785,7 +7787,7 @@ bool

>  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,

>  		     const_rtx *tocrel_offset_ret)

>  {

> -  if (!TARGET_TOC)

> +  if (!TARGET_TOC || TARGET_PCREL)


Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or
maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't
make much sense at all to have both enabled, does it?

> +      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,

> +	 we don't have to do any special for it.  */

> +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])

> +	       && TARGET_CMODEL == CMODEL_MEDIUM

> +	       && SYMBOL_REF_LOCAL_P (operands[1]))


      else if (TARGET_PCREL
	       && TARGET_CMODEL == CMODEL_MEDIUM
	       && SYMBOL_REF_P (operands[1])
	       && SYMBOL_REF_LOCAL_P (operands[1]))

> @@ -9928,6 +9938,11 @@ rs6000_emit_move (rtx dest, rtx source,

>  	      operands[1] = gen_const_mem (mode, tocref);

>  	      set_mem_alias_set (operands[1], get_TOC_alias_set ());

>  	    }

> +

> +	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))

> +		   && TARGET_CMODEL == CMODEL_MEDIUM

> +		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))

> +	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());


Similar.

>  alias_set_type

>  get_TOC_alias_set (void)

>  {

> -  if (set == -1)

> -    set = new_alias_set ();

> -  return set;

> +  if (TOC_alias_set == -1)

> +    TOC_alias_set = new_alias_set ();

> +  return TOC_alias_set;

> +}


It would be nice if you could initialise the alias sets some other way.
Not new code, but you duplicate it ;-)

> +alias_set_type

> +get_pc_relative_alias_set (void)

> +{

> +  if (pc_relative_alias_set == -1)

> +    pc_relative_alias_set = new_alias_set ();

> +  return pc_relative_alias_set;

>  }


Rest looks fine.


Segher
Michael Meissner July 11, 2019, 8:09 p.m. | #2
On Thu, Jul 11, 2019 at 02:52:03PM -0500, Segher Boessenkool wrote:
> On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:

> > +extern alias_set_type get_pc_relative_alias_set (void);

> 

> I'd just call it "pcrel", not "pc_relative", just like everywhere else?

> 

> > @@ -7785,7 +7787,7 @@ bool

> >  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,

> >  		     const_rtx *tocrel_offset_ret)

> >  {

> > -  if (!TARGET_TOC)

> > +  if (!TARGET_TOC || TARGET_PCREL)

> 

> Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or

> maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't

> make much sense at all to have both enabled, does it?


Yeah, in theory TARGET_TOC should not be set if TARGET_PCREL is set, but when I
last tried it, there were some failures.  I can make a macro for the two together.

> > +      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,

> > +	 we don't have to do any special for it.  */

> > +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])

> > +	       && TARGET_CMODEL == CMODEL_MEDIUM

> > +	       && SYMBOL_REF_LOCAL_P (operands[1]))

> 

>       else if (TARGET_PCREL

> 	       && TARGET_CMODEL == CMODEL_MEDIUM

> 	       && SYMBOL_REF_P (operands[1])

> 	       && SYMBOL_REF_LOCAL_P (operands[1]))

> 

> > @@ -9928,6 +9938,11 @@ rs6000_emit_move (rtx dest, rtx source,

> >  	      operands[1] = gen_const_mem (mode, tocref);

> >  	      set_mem_alias_set (operands[1], get_TOC_alias_set ());

> >  	    }

> > +

> > +	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))

> > +		   && TARGET_CMODEL == CMODEL_MEDIUM

> > +		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))

> > +	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());

> 

> Similar.


I had it in a function that did the checking, but after eliminating the
constant pool check that TOC needs, it was just doing the medium code model and
symbol_ref local checks, so I eliminated the function.

> >  alias_set_type

> >  get_TOC_alias_set (void)

> >  {

> > -  if (set == -1)

> > -    set = new_alias_set ();

> > -  return set;

> > +  if (TOC_alias_set == -1)

> > +    TOC_alias_set = new_alias_set ();

> > +  return TOC_alias_set;

> > +}

> 

> It would be nice if you could initialise the alias sets some other way.

> Not new code, but you duplicate it ;-)


For the pc-relative case, I'm not sure we need the alias set.  In the TOC case,
we do a force_const_mem which returns a SYMBOL_REF pointing to the data in the
constant pool.  Then we call create_TOC_reference which rewrites the memory,
and then it calls gen_const_mem which notes that memory in unchanging.

But for pc-relative, we just use output of force_const_mem.  Now
force_const_mem does not seem to set the alias set (but it will have the
constant pool bit set).

> > +alias_set_type

> > +get_pc_relative_alias_set (void)

> > +{

> > +  if (pc_relative_alias_set == -1)

> > +    pc_relative_alias_set = new_alias_set ();

> > +  return pc_relative_alias_set;

> >  }

> 

> Rest looks fine.

> 

> 

> Segher

> 


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Segher Boessenkool July 11, 2019, 9:36 p.m. | #3
On Thu, Jul 11, 2019 at 04:09:44PM -0400, Michael Meissner wrote:
> On Thu, Jul 11, 2019 at 02:52:03PM -0500, Segher Boessenkool wrote:

> > On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:

> > > +extern alias_set_type get_pc_relative_alias_set (void);

> > 

> > I'd just call it "pcrel", not "pc_relative", just like everywhere else?

> > 

> > > @@ -7785,7 +7787,7 @@ bool

> > >  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,

> > >  		     const_rtx *tocrel_offset_ret)

> > >  {

> > > -  if (!TARGET_TOC)

> > > +  if (!TARGET_TOC || TARGET_PCREL)

> > 

> > Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or

> > maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't

> > make much sense at all to have both enabled, does it?

> 

> Yeah, in theory TARGET_TOC should not be set if TARGET_PCREL is set, but when I

> last tried it, there were some failures.  I can make a macro for the two together.


TARGET_TOC is set in various header files (linux64.h etc.)  While
TARGET_PCREL is defined in rs6000.opt .  Maybe rename the original
TARGET_TOC definitions, and make a generic TARGET_TOC defined as
(TARGET_CAN_HAVE_TOC && !TARGET_PCREL) ?  Something like that?

> > > +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])

> > > +	       && TARGET_CMODEL == CMODEL_MEDIUM

> > > +	       && SYMBOL_REF_LOCAL_P (operands[1]))

> > 

> >       else if (TARGET_PCREL

> > 	       && TARGET_CMODEL == CMODEL_MEDIUM

> > 	       && SYMBOL_REF_P (operands[1])

> > 	       && SYMBOL_REF_LOCAL_P (operands[1]))


> I had it in a function that did the checking, but after eliminating the

> constant pool check that TOC needs, it was just doing the medium code model and

> symbol_ref local checks, so I eliminated the function.


My point is that you should put like things together, and you should not
put unrelated conditions on one line.

You could put the SYMBOL_REF_P and SYMBOL_REF_LOCAL_P on one line, I
wouldn't mind (except that doesn't fit on one line then ;-) ).

> > >  alias_set_type

> > >  get_TOC_alias_set (void)

> > >  {

> > > -  if (set == -1)

> > > -    set = new_alias_set ();

> > > -  return set;

> > > +  if (TOC_alias_set == -1)

> > > +    TOC_alias_set = new_alias_set ();

> > > +  return TOC_alias_set;

> > > +}

> > 

> > It would be nice if you could initialise the alias sets some other way.

> > Not new code, but you duplicate it ;-)

> 

> For the pc-relative case, I'm not sure we need the alias set.  In the TOC case,

> we do a force_const_mem which returns a SYMBOL_REF pointing to the data in the

> constant pool.  Then we call create_TOC_reference which rewrites the memory,

> and then it calls gen_const_mem which notes that memory in unchanging.

> 

> But for pc-relative, we just use output of force_const_mem.  Now

> force_const_mem does not seem to set the alias set (but it will have the

> constant pool bit set).


I mean just this:
  if (TOC_alias_set == -1)
    TOC_alias_set = new_alias_set ();
in the getter function.  It could be initialised elsewhere, there are
enough initialisation functions, it will fit *somewhere*, and then the
getter function doesn't need to do the initialisation anymore.  Which
then suddenly makes it very inlinable, etc.


Segher

Patch

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273361)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -190,6 +190,7 @@  extern void output_function_profiler (FI
 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_pc_relative_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 273363)
+++ 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))
@@ -7785,7 +7787,7 @@  bool
 toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
 		     const_rtx *tocrel_offset_ret)
 {
-  if (!TARGET_TOC)
+  if (!TARGET_TOC || TARGET_PCREL)
     return false;
 
   if (TARGET_CMODEL != CMODEL_SMALL)
@@ -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))
@@ -9865,10 +9867,18 @@  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
+      if (TARGET_TOC && !TARGET_PCREL
 	  && SYMBOL_REF_P (operands[1])
 	  && use_toc_relative_ref (operands[1], mode))
 	operands[1] = create_TOC_reference (operands[1], operands[0]);
+
+      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,
+	 we don't have to do any special for it.  */
+      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
+	       && TARGET_CMODEL == CMODEL_MEDIUM
+	       && SYMBOL_REF_LOCAL_P (operands[1]))
+	;
+
       else if (mode == Pmode
 	       && CONSTANT_P (operands[1])
 	       && GET_CODE (operands[1]) != HIGH
@@ -9919,7 +9929,7 @@  rs6000_emit_move (rtx dest, rtx source,
 
 	  operands[1] = force_const_mem (mode, operands[1]);
 
-	  if (TARGET_TOC
+	  if (TARGET_TOC && !TARGET_PCREL
 	      && SYMBOL_REF_P (XEXP (operands[1], 0))
 	      && use_toc_relative_ref (XEXP (operands[1], 0), mode))
 	    {
@@ -9928,6 +9938,11 @@  rs6000_emit_move (rtx dest, rtx source,
 	      operands[1] = gen_const_mem (mode, tocref);
 	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
 	    }
+
+	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))
+		   && TARGET_CMODEL == CMODEL_MEDIUM
+		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))
+	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());
 	}
       break;
 
@@ -23732,14 +23747,26 @@  rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+/* Alias set for constants accessed via the TOC.  */
+static GTY(()) alias_set_type TOC_alias_set = -1;
 
 alias_set_type
 get_TOC_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (TOC_alias_set == -1)
+    TOC_alias_set = new_alias_set ();
+  return TOC_alias_set;
+}
+
+/* Alias set for constants accessed via the pc-relative addressing.  */
+static GTY(()) alias_set_type pc_relative_alias_set = -1;
+
+alias_set_type
+get_pc_relative_alias_set (void)
+{
+  if (pc_relative_alias_set == -1)
+    pc_relative_alias_set = new_alias_set ();
+  return pc_relative_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 273361)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11606,7 +11606,7 @@  (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_TOC && !TARGET_PCREL)
     {
       rtx tocref;
       tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]);
@@ -11616,6 +11616,12 @@  (define_insn_and_split "*cmp<mode>_inter
       set_mem_alias_set (operands[14], get_TOC_alias_set ());
       set_mem_alias_set (operands[15], get_TOC_alias_set ());
     }
+
+  else if (TARGET_PCREL)
+    {
+      set_mem_alias_set (operands[14], get_pc_relative_alias_set ());
+      set_mem_alias_set (operands[15], get_pc_relative_alias_set ());
+    }
 })
 
 ;; Now we have the scc insns.  We can do some combinations because of the