Re: [PATCH rs6000] remove implicit static var outputs of toc_relative_expr_p

[Aaron Sawdey <acsawdey at linux dot vnet dot ibm dot com>]

Hi Segher,

On Tue, 2017-06-27 at 18:35 -0500, Segher Boessenkool wrote:
> Hi Aaron,
> 
> On Tue, Jun 27, 2017 at 11:43:57AM -0500, Aaron Sawdey wrote:
> > The function toc_relative_expr_p implicitly sets two static vars
> > (tocrel_base and tocrel_offset) that are declared in rs6000.c. The
> > real
> > purpose of this is to communicate between
> > print_operand/print_operand_address and
> > rs6000_output_addr_const_extra,
> > which is called through the asm_out hook vector by something in the
> > call tree under output_addr_const.
> > 
> > This patch changes toc_relative_expr_p to make tocrel_base and
> > tocrel_offset be explicit const_rtx * args. All of the calls other
> > than
> > print_operand/print_operand_address are changed to have local
> > const_rtx
> > vars that are passed in.
> 
> If those locals aren't used, can you arrange to call
> toc_relative_expr_p
> with NULL instead?  Or are they always used?

There are calls where neither is used or only tocrel_base is used.

> 
> > The statics in rs6000.c are now called
> > tocrel_base_oac and tocrel_offset_oac to reflect their use to
> > communicate across output_addr_const, and that is now the only
> > thing
> > they are used for.
> 
> Can't say I like those names, very cryptical.  Not that I know
> something
> better, the short names as they were weren't very nice either.
> 
> > --- gcc/config/rs6000/rs6000.c	(revision 249639)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -8628,18 +8628,25 @@
> >  	  && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant
> > (base), Pmode));
> >  }
> >  
> > -static const_rtx tocrel_base, tocrel_offset;
> > +/* These are only used to pass through from
> > print_operand/print_operand_address
> > + * to rs6000_output_addr_const_extra over the intervening
> > function 
> > + * output_addr_const which is not target code.  */
> 
> No leading * in a block comment please.  (And you have a trailing
> space).
> 
> > +static const_rtx tocrel_base_oac, tocrel_offset_oac;
> >  
> >  /* Return true if OP is a toc pointer relative address (the output
> >     of create_TOC_reference).  If STRICT, do not match non-split
> > -   -mcmodel=large/medium toc pointer relative addresses.  */
> > +   -mcmodel=large/medium toc pointer relative addresses.  Places
> > base 
> > +   and offset pieces in TOCREL_BASE and TOCREL_OFFSET
> > respectively.  */
> 
> s/Places/Place/ (and another trailing space).
> 
> > -  tocrel_base = op;
> > -  tocrel_offset = const0_rtx;
> > +  *tocrel_base = op;
> > +  *tocrel_offset = const0_rtx;
> >    if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1),
> > GET_MODE (op)))
> >      {
> > -      tocrel_base = XEXP (op, 0);
> > -      tocrel_offset = XEXP (op, 1);
> > +      *tocrel_base = XEXP (op, 0);
> > +      *tocrel_offset = XEXP (op, 1);
> >      }
> 
> Maybe write this as
> 
>   if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1),
> GET_MODE (op)))
>     {
>       *tocrel_base = XEXP (op, 0);
>       *tocrel_offset = XEXP (op, 1);
>     }
>   else
>     {
>       *tocrel_base = op;
>       *tocrel_offset = const0_rtx;
>     }
> 
> or, if you allow NULL pointers,
> 
>   bool with_offset = GET_CODE (op) == PLUS
> 		     && add_cint_operand (XEXP (op, 1), GET_MODE (op));
>   if (tocrel_base)
>     *tocrel_base = with_offset ? XEXP (op, 0) : op;
>   if (tocrel_offset)
>     *tocrel_offset = with_offset ? XEXP (op, 1) : const0_rtx;
> 
> or such.
> 
> > -  return (GET_CODE (tocrel_base) == UNSPEC
> > -	  && XINT (tocrel_base, 1) == UNSPEC_TOCREL);
> > +  return (GET_CODE (*tocrel_base) == UNSPEC
> > +	  && XINT (*tocrel_base, 1) == UNSPEC_TOCREL);
> 
> Well, and then you have this, so you need to assign tocrel_base to a
> local
> as well.
> 
> >  legitimate_constant_pool_address_p (const_rtx x, machine_mode
> > mode,
> >  				    bool strict)
> >  {
> > -  return (toc_relative_expr_p (x, strict)
> > +  const_rtx tocrel_base, tocrel_offset;
> > +  return (toc_relative_expr_p (x, strict, &tocrel_base,
> > &tocrel_offset)
> 
> For example here it seems nothing uses tocrel_base?
> 
> It is probably nicer to have a separate function for
> toc_relative_expr_p
> and one to pull the base/offset out.  And maybe don't keep it cached
> for
> the output function either?  It has all info it needs, right, the
> full
> address RTX?  I don't think it is measurably slower to pull the
> address
> apart an extra time?

I think it doesn't make a lot of sense to have two functions as you
have to do nearly all the work just to get the true/false return value,
you have to completely compute tocrel_base. I've restructured this so
that either pointer can be null. The function uses locals for
tocrel_base/tocrel_offset, then assigns to the pointers if non-null.

bool
toc_relative_expr_p (const_rtx op, bool strict, const_rtx
*tocrel_base_ret,
		     const_rtx *tocrel_offset_ret)
{
  if (!TARGET_TOC)
    return false;

  if (TARGET_CMODEL != CMODEL_SMALL)
    {
      /* When strict ensure we have everything tidy.  */
      if (strict
	  && !(GET_CODE (op) == LO_SUM
	       && REG_P (XEXP (op, 0))
	       && INT_REG_OK_FOR_BASE_P (XEXP (op, 0), strict)))
	return false;

      /* When not strict, allow non-split TOC addresses and also allow
	 (lo_sum (high ..)) TOC addresses created during reload.  */
      if (GET_CODE (op) == LO_SUM)
	op = XEXP (op, 1);
    }

  const_rtx tocrel_base = op;
  const_rtx tocrel_offset = const0_rtx;

  if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE
(op)))
    {
      tocrel_base = XEXP (op, 0);
      if (tocrel_offset_ret)
	tocrel_offset = XEXP (op, 1);
    }

  if (tocrel_base_ret)
    *tocrel_base_ret = tocrel_base;
  if (tocrel_offset_ret)
    *tocrel_offset_ret = tocrel_offset;

  return (GET_CODE (tocrel_base) == UNSPEC
	  && XINT (tocrel_base, 1) == UNSPEC_TOCREL);
}

Revised patch is attached, bootstrap/regtest in progress. If everything
passes, ok for trunk?

Thanks,
    Aaron

-- 
Aaron Sawdey, Ph.D.  acsawdey@linux.vnet.ibm.com
050-2/C113  (507) 253-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 249639)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -40,7 +40,7 @@
 extern int small_data_operand (rtx, machine_mode);
 extern bool mem_operand_gpr (rtx, machine_mode);
 extern bool mem_operand_ds_form (rtx, machine_mode);
-extern bool toc_relative_expr_p (const_rtx, bool);
+extern bool toc_relative_expr_p (const_rtx, bool, const_rtx *, const_rtx *);
 extern void validate_condition_mode (enum rtx_code, machine_mode);
 extern bool legitimate_constant_pool_address_p (const_rtx, machine_mode,
 						bool);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 249639)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8628,14 +8628,19 @@
 	  && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (base), Pmode));
 }
 
-static const_rtx tocrel_base, tocrel_offset;
+/* These are only used to pass through from print_operand/print_operand_address
+   to rs6000_output_addr_const_extra over the intervening function
+   output_addr_const which is not target code.  */
+static const_rtx tocrel_base_oac, tocrel_offset_oac;
 
 /* Return true if OP is a toc pointer relative address (the output
    of create_TOC_reference).  If STRICT, do not match non-split
-   -mcmodel=large/medium toc pointer relative addresses.  */
+   -mcmodel=large/medium toc pointer relative addresses.  Place base
+   and offset pieces in TOCREL_BASE and TOCREL_OFFSET respectively.  */
 
 bool
-toc_relative_expr_p (const_rtx op, bool strict)
+toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
+		     const_rtx *tocrel_offset_ret)
 {
   if (!TARGET_TOC)
     return false;
@@ -8655,14 +8660,21 @@
 	op = XEXP (op, 1);
     }
 
-  tocrel_base = op;
-  tocrel_offset = const0_rtx;
+  const_rtx tocrel_base = op;
+  const_rtx tocrel_offset = const0_rtx;
+
   if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op)))
     {
       tocrel_base = XEXP (op, 0);
-      tocrel_offset = XEXP (op, 1);
+      if (tocrel_offset_ret)
+	tocrel_offset = XEXP (op, 1);
     }
 
+  if (tocrel_base_ret)
+    *tocrel_base_ret = tocrel_base;
+  if (tocrel_offset_ret)
+    *tocrel_offset_ret = tocrel_offset;
+
   return (GET_CODE (tocrel_base) == UNSPEC
 	  && XINT (tocrel_base, 1) == UNSPEC_TOCREL);
 }
@@ -8674,7 +8686,8 @@
 legitimate_constant_pool_address_p (const_rtx x, machine_mode mode,
 				    bool strict)
 {
-  return (toc_relative_expr_p (x, strict)
+  const_rtx tocrel_base, tocrel_offset;
+  return (toc_relative_expr_p (x, strict, &tocrel_base, &tocrel_offset)
 	  && (TARGET_CMODEL != CMODEL_MEDIUM
 	      || constant_pool_expr_p (XVECEXP (tocrel_base, 0, 0))
 	      || mode == QImode
@@ -11069,7 +11082,7 @@
 			   > (TARGET_CMODEL != CMODEL_SMALL ? 3 : 2)))
 		   || (GET_CODE (operands[0]) == REG
 		       && FP_REGNO_P (REGNO (operands[0]))))
-	       && !toc_relative_expr_p (operands[1], false)
+	       && !toc_relative_expr_p (operands[1], false, NULL, NULL)
 	       && (TARGET_CMODEL == CMODEL_SMALL
 		   || can_create_pseudo_p ()
 		   || (REG_P (operands[0])
@@ -21812,7 +21825,7 @@
 	}
       else
 	{
-	  if (toc_relative_expr_p (x, false))
+	  if (toc_relative_expr_p (x, false, &tocrel_base_oac, &tocrel_offset_oac))
 	    /* This hack along with a corresponding hack in
 	       rs6000_output_addr_const_extra arranges to output addends
 	       where the assembler expects to find them.  eg.
@@ -21819,7 +21832,7 @@
 	       (plus (unspec [(symbol_ref ("x")) (reg 2)] tocrel) 4)
 	       without this hack would be output as "x@toc+4".  We
 	       want "x+4@toc".  */
-	    output_addr_const (file, CONST_CAST_RTX (tocrel_base));
+	    output_addr_const (file, CONST_CAST_RTX (tocrel_base_oac));
 	  else
 	    output_addr_const (file, x);
 	}
@@ -21886,7 +21899,7 @@
       fprintf (file, "@l(%s)", reg_names[ REGNO (XEXP (x, 0)) ]);
     }
 #endif
-  else if (toc_relative_expr_p (x, false))
+  else if (toc_relative_expr_p (x, false, &tocrel_base_oac, &tocrel_offset_oac))
     {
       /* This hack along with a corresponding hack in
 	 rs6000_output_addr_const_extra arranges to output addends
@@ -21895,17 +21908,17 @@
 	 .       (plus (unspec [(symbol_ref ("x")) (reg 2)] tocrel) 8))
 	 without this hack would be output as "x@toc+8@l(9)".  We
 	 want "x+8@toc@l(9)".  */
-      output_addr_const (file, CONST_CAST_RTX (tocrel_base));
+      output_addr_const (file, CONST_CAST_RTX (tocrel_base_oac));
       if (GET_CODE (x) == LO_SUM)
 	fprintf (file, "@l(%s)", reg_names[REGNO (XEXP (x, 0))]);
       else
-	fprintf (file, "(%s)", reg_names[REGNO (XVECEXP (tocrel_base, 0, 1))]);
+	fprintf (file, "(%s)", reg_names[REGNO (XVECEXP (tocrel_base_oac, 0, 1))]);
     }
   else
     gcc_unreachable ();
 }
 
-/* Implement TARGET_OUTPUT_ADDR_CONST_EXTRA.  */
+/* Implement TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA.  */
 
 static bool
 rs6000_output_addr_const_extra (FILE *file, rtx x)
@@ -21918,11 +21931,11 @@
 			     && REG_P (XVECEXP (x, 0, 1))
 			     && REGNO (XVECEXP (x, 0, 1)) == TOC_REGISTER);
 	output_addr_const (file, XVECEXP (x, 0, 0));
-	if (x == tocrel_base && tocrel_offset != const0_rtx)
+	if (x == tocrel_base_oac && tocrel_offset_oac != const0_rtx)
 	  {
-	    if (INTVAL (tocrel_offset) >= 0)
+	    if (INTVAL (tocrel_offset_oac) >= 0)
 	      fprintf (file, "+");
-	    output_addr_const (file, CONST_CAST_RTX (tocrel_offset));
+	    output_addr_const (file, CONST_CAST_RTX (tocrel_offset_oac));
 	  }
 	if (!TARGET_AIX || (TARGET_ELF && TARGET_MINIMAL_TOC))
 	  {
@@ -39312,6 +39325,8 @@
   if (!insn_entry[uid].is_swap || insn_entry[uid].is_load)
     return false;
 
+  const_rtx tocrel_base;
+
   /* Find the unique use in the swap and locate its def.  If the def
      isn't unique, punt.  */
   struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
@@ -39357,7 +39372,7 @@
 	  rtx tocrel_expr = SET_SRC (tocrel_body);
 	  if (GET_CODE (tocrel_expr) == MEM)
 	    tocrel_expr = XEXP (tocrel_expr, 0);
-	  if (!toc_relative_expr_p (tocrel_expr, false))
+	  if (!toc_relative_expr_p (tocrel_expr, false, &tocrel_base, NULL))
 	    return false;
 	  split_const (XVECEXP (tocrel_base, 0, 0), &base, &offset);
 	  if (GET_CODE (base) != SYMBOL_REF || !CONSTANT_POOL_ADDRESS_P (base))
@@ -40118,11 +40133,12 @@
      to set tocrel_base; otherwise it would be unnecessary as we've
      already established it will return true.  */
   rtx base, offset;
+  const_rtx tocrel_base;
   rtx tocrel_expr = SET_SRC (PATTERN (tocrel_insn));
   /* There is an extra level of indirection for small/large code models.  */
   if (GET_CODE (tocrel_expr) == MEM)
     tocrel_expr = XEXP (tocrel_expr, 0);
-  if (!toc_relative_expr_p (tocrel_expr, false))
+  if (!toc_relative_expr_p (tocrel_expr, false, &tocrel_base, NULL))
     gcc_unreachable ();
   split_const (XVECEXP (tocrel_base, 0, 0), &base, &offset);
   rtx const_vector = get_pool_constant (base);