diff mbox series

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

Commit Message

Michael Meissner July 10, 2019, 9:17 p.m. UTC 
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.

Comments

Segher Boessenkool July 11, 2019, 7:52 p.m. UTC | #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. UTC | #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
>
Segher Boessenkool July 11, 2019, 9:36 p.m. UTC | #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
diff mbox series

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