[PR,debug/83758] look more carefully for internal_arg_pointer in vt_add_function_parameter()
diff mbox series

Message ID 1517259670.4150.19.camel@linux.vnet.ibm.com
State New
Headers show
Series
  • [PR,debug/83758] look more carefully for internal_arg_pointer in vt_add_function_parameter()
Related show

Commit Message

Aaron Sawdey Jan. 29, 2018, 9:01 p.m. UTC
This bug appears to revolve around whether there is a canonical rtx for
internal_arg_pointer in var-tracking. In vt_add_function_parameter() we
 currently have:

static void
vt_add_function_parameter (tree parm)
{
  rtx decl_rtl = DECL_RTL_IF_SET (parm);
  rtx incoming = DECL_INCOMING_RTL (parm);
  tree decl;
  machine_mode mode;
  poly_int64 offset;
  dataflow_set *out;
  decl_or_value dv;

  if (TREE_CODE (parm) != PARM_DECL)
    return;

  if (!decl_rtl || !incoming)
    return;

  if (GET_MODE (decl_rtl) == BLKmode || GET_MODE (incoming) == BLKmode)
    return;

  /* If there is a DRAP register or a pseudo in internal_arg_pointer,
     rewrite the incoming location of parameters passed on the stack
     into MEMs based on the argument pointer, so that incoming doesn't
     depend on a pseudo.  */
  if (MEM_P (incoming)
      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
	      && XEXP (XEXP (incoming, 0), 0)
		 == crtl->args.internal_arg_pointer
	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
    {
      HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
      if (GET_CODE (XEXP (incoming, 0)) == PLUS)
	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
      incoming
	= replace_equiv_address_nv (incoming,
				    plus_constant (Pmode,
						   arg_pointer_rtx, off));
    }


This is looking for crtl->args.internal_arg_pointer within rtx
incoming. The problem I am seeing is that the same rtx is there, just
not as a pointer to the identical rtx objects, so is not found by the
== comparison in the current code. So hence my patch below to switch
from == to rtx_equal_p(). If the expression is not rewritten, then the
pseudo created for the stack pointer is not preserved and later we run
into the assert near the beginning of vt_expand_var_loc_chain().

Bootstrap now passes for languages=c,c++,go on ppc64le. If
bootstrap/regtest is ok on ppc64le and x86_64, ok for trunk?


2018-01-29  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>

	* var-tracking.c (vt_add_function_parameter): Fix comparison of rtx.


       HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);

Comments

Jakub Jelinek Jan. 29, 2018, 10:05 p.m. UTC | #1
On Mon, Jan 29, 2018 at 03:01:10PM -0600, Aaron Sawdey wrote:
>   /* If there is a DRAP register or a pseudo in internal_arg_pointer,
>      rewrite the incoming location of parameters passed on the stack
>      into MEMs based on the argument pointer, so that incoming doesn't
>      depend on a pseudo.  */
>   if (MEM_P (incoming)
>       && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
> 	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
> 	      && XEXP (XEXP (incoming, 0), 0)
> 		 == crtl->args.internal_arg_pointer
> 	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
>     {
>       HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
>       if (GET_CODE (XEXP (incoming, 0)) == PLUS)
> 	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
>       incoming
> 	= replace_equiv_address_nv (incoming,
> 				    plus_constant (Pmode,
> 						   arg_pointer_rtx, off));
>     }

The code actually meant pointer comparison, the question is what is
different on powerpc* that you end up with a different REG.
From what I can see, function.c uses crtl->args.internal_arg_pointer
directly rather than a REG with the same REGNO.
Where does it become something different and why?

	Jakub
Segher Boessenkool Jan. 29, 2018, 10:26 p.m. UTC | #2
On Mon, Jan 29, 2018 at 11:05:29PM +0100, Jakub Jelinek wrote:
> On Mon, Jan 29, 2018 at 03:01:10PM -0600, Aaron Sawdey wrote:
> >   /* If there is a DRAP register or a pseudo in internal_arg_pointer,
> >      rewrite the incoming location of parameters passed on the stack
> >      into MEMs based on the argument pointer, so that incoming doesn't
> >      depend on a pseudo.  */
> >   if (MEM_P (incoming)
> >       && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
> > 	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
> > 	      && XEXP (XEXP (incoming, 0), 0)
> > 		 == crtl->args.internal_arg_pointer
> > 	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
> >     {
> >       HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
> >       if (GET_CODE (XEXP (incoming, 0)) == PLUS)
> > 	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
> >       incoming
> > 	= replace_equiv_address_nv (incoming,
> > 				    plus_constant (Pmode,
> > 						   arg_pointer_rtx, off));
> >     }
> 
> The code actually meant pointer comparison, the question is what is
> different on powerpc* that you end up with a different REG.
> >From what I can see, function.c uses crtl->args.internal_arg_pointer
> directly rather than a REG with the same REGNO.
> Where does it become something different and why?

There is a lot of code that copies any RTX that isn't obviously unique.
Here we have a PLUS of some things, which always needs copying, can
never be shared.

I agree internal_arg_pointer should be a register: documentation says so.
(Well actually it doesn't, there is no documentation for it, but the
next best thing, the header file where it is declared, says so).

But rs6000's implementation worked for years, maybe we shouldn't break
it now?  Or maybe it is easy to fix.  Or, does that restriction ("has
to be a pseudo or hard reg") actually buy us anything?


Segher
Jakub Jelinek Jan. 29, 2018, 10:41 p.m. UTC | #3
On Mon, Jan 29, 2018 at 04:26:50PM -0600, Segher Boessenkool wrote:
> > The code actually meant pointer comparison, the question is what is
> > different on powerpc* that you end up with a different REG.
> > >From what I can see, function.c uses crtl->args.internal_arg_pointer
> > directly rather than a REG with the same REGNO.
> > Where does it become something different and why?
> 
> There is a lot of code that copies any RTX that isn't obviously unique.
> Here we have a PLUS of some things, which always needs copying, can
> never be shared.

Yes, PLUS needs to be unshared.
So it ought to be handled by the PLUS handling code.
          || (GET_CODE (XEXP (incoming, 0)) == PLUS
              && XEXP (XEXP (incoming, 0), 0)
                 == crtl->args.internal_arg_pointer
              && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
Here we are talking about MEMs on the PARM_DECL DECL_RTLs, those are not
instantiated as normal IL in the RTL stream is.

I'm not saying the var-tracking.c change is wrong, but I'd like to see
analysis on what is going on.

Is the patch for -fsplit-stack where rs6000_internal_arg_pointer
returns a PLUS of some pseudo and some offset?
In that case I wonder how does the patch with rtx_equal_p actually work,
because function.c then uses plus_constant on this result, and if there are
2 non-zero offsets added, the result is a PLUS with a single offset based
on that pseudo.  That would mean var-tracking.c would need special code to
handle crtl->args.internal_arg_pointer that is a PLUS, and handle it
differently from crtl->args.internal_arg_pointer that is a pseudo REG.

	Jakub
Segher Boessenkool Jan. 29, 2018, 11:09 p.m. UTC | #4
On Mon, Jan 29, 2018 at 11:41:32PM +0100, Jakub Jelinek wrote:
> On Mon, Jan 29, 2018 at 04:26:50PM -0600, Segher Boessenkool wrote:
> > > The code actually meant pointer comparison, the question is what is
> > > different on powerpc* that you end up with a different REG.
> > > >From what I can see, function.c uses crtl->args.internal_arg_pointer
> > > directly rather than a REG with the same REGNO.
> > > Where does it become something different and why?
> > 
> > There is a lot of code that copies any RTX that isn't obviously unique.
> > Here we have a PLUS of some things, which always needs copying, can
> > never be shared.
> 
> Yes, PLUS needs to be unshared.
> So it ought to be handled by the PLUS handling code.
>           || (GET_CODE (XEXP (incoming, 0)) == PLUS
>               && XEXP (XEXP (incoming, 0), 0)
>                  == crtl->args.internal_arg_pointer
>               && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
> Here we are talking about MEMs on the PARM_DECL DECL_RTLs, those are not
> instantiated as normal IL in the RTL stream is.

This is a PLUS of something with the internal_arg_pointer.  Our
internal_arg_pointer _itself_ is a PLUS, so it should be copy_rtx'd
everywhere it is used (or risk RTL sharing).

> I'm not saying the var-tracking.c change is wrong, but I'd like to see
> analysis on what is going on.

The rtx_equal_p is equal to the == for anything that uses just a single
register.  For us it makes the compiler bootstrap again (with Go enabled),
not unnice to have in stage4 ;-)

I agree the code does not seem to be set up for PLUS to work here.

> Is the patch for -fsplit-stack where rs6000_internal_arg_pointer
> returns a PLUS of some pseudo and some offset?

Yeah.

> In that case I wonder how does the patch with rtx_equal_p actually work,
> because function.c then uses plus_constant on this result, and if there are

Not everywhere, in places it does

  gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);

(so we end up with non-canonical RTL).

> 2 non-zero offsets added, the result is a PLUS with a single offset based
> on that pseudo.  That would mean var-tracking.c would need special code to
> handle crtl->args.internal_arg_pointer that is a PLUS, and handle it
> differently from crtl->args.internal_arg_pointer that is a pseudo REG.

Yeah.  Not entirely pleasant.  Not super hard for just reg+off, but for
general expressions it isn't very funny.

If we want to force internal_arg_pointer to be just one reg the doc in
target.def should be updated:

/* ??? Documenting this hook requires a GFDL license grant.  */
DEFHOOK_UNDOC
(internal_arg_pointer,
"Return an rtx for the argument pointer incoming to the\
 current function.",
 rtx, (void),
 default_internal_arg_pointer)


Segher
Richard Sandiford Jan. 30, 2018, 9:41 a.m. UTC | #5
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Jan 29, 2018 at 11:41:32PM +0100, Jakub Jelinek wrote:
>> On Mon, Jan 29, 2018 at 04:26:50PM -0600, Segher Boessenkool wrote:
>> > > The code actually meant pointer comparison, the question is what is
>> > > different on powerpc* that you end up with a different REG.
>> > > >From what I can see, function.c uses crtl->args.internal_arg_pointer
>> > > directly rather than a REG with the same REGNO.
>> > > Where does it become something different and why?
>> > 
>> > There is a lot of code that copies any RTX that isn't obviously unique.
>> > Here we have a PLUS of some things, which always needs copying, can
>> > never be shared.
>> 
>> Yes, PLUS needs to be unshared.
>> So it ought to be handled by the PLUS handling code.
>>           || (GET_CODE (XEXP (incoming, 0)) == PLUS
>>               && XEXP (XEXP (incoming, 0), 0)
>>                  == crtl->args.internal_arg_pointer
>>               && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
>> Here we are talking about MEMs on the PARM_DECL DECL_RTLs, those are not
>> instantiated as normal IL in the RTL stream is.
>
> This is a PLUS of something with the internal_arg_pointer.  Our
> internal_arg_pointer _itself_ is a PLUS, so it should be copy_rtx'd
> everywhere it is used (or risk RTL sharing).

Is that needed even in DECL_RTL?

>> I'm not saying the var-tracking.c change is wrong, but I'd like to see
>> analysis on what is going on.
>
> The rtx_equal_p is equal to the == for anything that uses just a single
> register.  For us it makes the compiler bootstrap again (with Go enabled),
> not unnice to have in stage4 ;-)
>
> I agree the code does not seem to be set up for PLUS to work here.
>
>> Is the patch for -fsplit-stack where rs6000_internal_arg_pointer
>> returns a PLUS of some pseudo and some offset?
>
> Yeah.
>
>> In that case I wonder how does the patch with rtx_equal_p actually work,
>> because function.c then uses plus_constant on this result, and if there are
>
> Not everywhere, in places it does
>
>   gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
>
> (so we end up with non-canonical RTL).

But in that case, what does the copying?

That's what seems strange.  I can see why we'd have two nested
pluses with the inner plus being pointer-equal to internal_arg_ptr.
And I can see why we'd have a single canonical plus (which IMO would
be better, but I agree it's not stage 4 material).  It's having the two
nested pluses without maintaining pointer equality that seems strange.

Thanks,
Richard
Segher Boessenkool Jan. 30, 2018, 9:55 a.m. UTC | #6
On Tue, Jan 30, 2018 at 09:41:47AM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Mon, Jan 29, 2018 at 11:41:32PM +0100, Jakub Jelinek wrote:
> >> On Mon, Jan 29, 2018 at 04:26:50PM -0600, Segher Boessenkool wrote:
> >> > > The code actually meant pointer comparison, the question is what is
> >> > > different on powerpc* that you end up with a different REG.
> >> > > >From what I can see, function.c uses crtl->args.internal_arg_pointer
> >> > > directly rather than a REG with the same REGNO.
> >> > > Where does it become something different and why?
> >> > 
> >> > There is a lot of code that copies any RTX that isn't obviously unique.
> >> > Here we have a PLUS of some things, which always needs copying, can
> >> > never be shared.
> >> 
> >> Yes, PLUS needs to be unshared.
> >> So it ought to be handled by the PLUS handling code.
> >>           || (GET_CODE (XEXP (incoming, 0)) == PLUS
> >>               && XEXP (XEXP (incoming, 0), 0)
> >>                  == crtl->args.internal_arg_pointer
> >>               && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
> >> Here we are talking about MEMs on the PARM_DECL DECL_RTLs, those are not
> >> instantiated as normal IL in the RTL stream is.
> >
> > This is a PLUS of something with the internal_arg_pointer.  Our
> > internal_arg_pointer _itself_ is a PLUS, so it should be copy_rtx'd
> > everywhere it is used (or risk RTL sharing).
> 
> Is that needed even in DECL_RTL?

When it is copied to the RTL stream, it has to yes.  No sharing allowed,
even if nothing inside this is ever modified.

> >> I'm not saying the var-tracking.c change is wrong, but I'd like to see
> >> analysis on what is going on.
> >
> > The rtx_equal_p is equal to the == for anything that uses just a single
> > register.  For us it makes the compiler bootstrap again (with Go enabled),
> > not unnice to have in stage4 ;-)
> >
> > I agree the code does not seem to be set up for PLUS to work here.
> >
> >> Is the patch for -fsplit-stack where rs6000_internal_arg_pointer
> >> returns a PLUS of some pseudo and some offset?
> >
> > Yeah.
> >
> >> In that case I wonder how does the patch with rtx_equal_p actually work,
> >> because function.c then uses plus_constant on this result, and if there are
> >
> > Not everywhere, in places it does
> >
> >   gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
> >
> > (so we end up with non-canonical RTL).
> 
> But in that case, what does the copying?

I don't know.  Aaron will look at it, but timezones etc. :-)

> That's what seems strange.  I can see why we'd have two nested
> pluses with the inner plus being pointer-equal to internal_arg_ptr.
> And I can see why we'd have a single canonical plus (which IMO would
> be better, but I agree it's not stage 4 material).  It's having the two
> nested pluses without maintaining pointer equality that seems strange.

The inner plus is *not* pointer-equal, that is the problem.  Something
did copy_rtx (or such) on it, many things do.  We can tell you what
exactly later today.


Segher
Richard Sandiford Jan. 30, 2018, 10:02 a.m. UTC | #7
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Jan 30, 2018 at 09:41:47AM +0000, Richard Sandiford wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Mon, Jan 29, 2018 at 11:41:32PM +0100, Jakub Jelinek wrote:
>> >> On Mon, Jan 29, 2018 at 04:26:50PM -0600, Segher Boessenkool wrote:
>> >> > > The code actually meant pointer comparison, the question is what is
>> >> > > different on powerpc* that you end up with a different REG.
>> >> > > >From what I can see, function.c uses crtl->args.internal_arg_pointer
>> >> > > directly rather than a REG with the same REGNO.
>> >> > > Where does it become something different and why?
>> >> > 
>> >> > There is a lot of code that copies any RTX that isn't obviously unique.
>> >> > Here we have a PLUS of some things, which always needs copying, can
>> >> > never be shared.
>> >> 
>> >> Yes, PLUS needs to be unshared.
>> >> So it ought to be handled by the PLUS handling code.
>> >>           || (GET_CODE (XEXP (incoming, 0)) == PLUS
>> >>               && XEXP (XEXP (incoming, 0), 0)
>> >>                  == crtl->args.internal_arg_pointer
>> >>               && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
>> >> Here we are talking about MEMs on the PARM_DECL DECL_RTLs, those are not
>> >> instantiated as normal IL in the RTL stream is.
>> >
>> > This is a PLUS of something with the internal_arg_pointer.  Our
>> > internal_arg_pointer _itself_ is a PLUS, so it should be copy_rtx'd
>> > everywhere it is used (or risk RTL sharing).
>> 
>> Is that needed even in DECL_RTL?
>
> When it is copied to the RTL stream, it has to yes.  No sharing allowed,
> even if nothing inside this is ever modified.

Right.  But DECL_RTL isn't in the RTL stream.  It needs to be copied
before being used there.

And this code is looking at DECL_RTL, not at rtl stream (insn patterns).

>> >> I'm not saying the var-tracking.c change is wrong, but I'd like to see
>> >> analysis on what is going on.
>> >
>> > The rtx_equal_p is equal to the == for anything that uses just a single
>> > register.  For us it makes the compiler bootstrap again (with Go enabled),
>> > not unnice to have in stage4 ;-)
>> >
>> > I agree the code does not seem to be set up for PLUS to work here.
>> >
>> >> Is the patch for -fsplit-stack where rs6000_internal_arg_pointer
>> >> returns a PLUS of some pseudo and some offset?
>> >
>> > Yeah.
>> >
>> >> In that case I wonder how does the patch with rtx_equal_p actually work,
>> >> because function.c then uses plus_constant on this result, and if there are
>> >
>> > Not everywhere, in places it does
>> >
>> >   gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
>> >
>> > (so we end up with non-canonical RTL).
>> 
>> But in that case, what does the copying?
>
> I don't know.  Aaron will look at it, but timezones etc. :-)
>
>> That's what seems strange.  I can see why we'd have two nested
>> pluses with the inner plus being pointer-equal to internal_arg_ptr.
>> And I can see why we'd have a single canonical plus (which IMO would
>> be better, but I agree it's not stage 4 material).  It's having the two
>> nested pluses without maintaining pointer equality that seems strange.
>
> The inner plus is *not* pointer-equal, that is the problem.  Something
> did copy_rtx (or such) on it, many things do.  We can tell you what
> exactly later today.

Yeah, I realise that.  And I'm saying that's what seems strange :-)
If something is deliberately avoiding folding the plus so that we
can still see internal_arg_ptr, why does it end up copying the
internal_arg_ptr regardless?

Thanks,
Richard
Jakub Jelinek Jan. 30, 2018, 10:13 a.m. UTC | #8
On Tue, Jan 30, 2018 at 03:55:58AM -0600, Segher Boessenkool wrote:
> > But in that case, what does the copying?
> 
> I don't know.  Aaron will look at it, but timezones etc. :-)
> 
> > That's what seems strange.  I can see why we'd have two nested
> > pluses with the inner plus being pointer-equal to internal_arg_ptr.
> > And I can see why we'd have a single canonical plus (which IMO would
> > be better, but I agree it's not stage 4 material).  It's having the two
> > nested pluses without maintaining pointer equality that seems strange.
> 
> The inner plus is *not* pointer-equal, that is the problem.  Something
> did copy_rtx (or such) on it, many things do.  We can tell you what
> exactly later today.

Most likely unshare_all_rtl, which does:
  for (tree decl = DECL_ARGUMENTS (cfun->decl); decl; decl = DECL_CHAIN (decl))
    {
      if (DECL_RTL_SET_P (decl))
        SET_DECL_RTL (decl, copy_rtx_if_shared (DECL_RTL (decl)));
      DECL_INCOMING_RTL (decl) = copy_rtx_if_shared (DECL_INCOMING_RTL (decl));
    }

Anyway, my preference would be to change that gen_rtx_PLUS into
  stack_parm = crtl->args.internal_arg_pointer;
  if (!CONST_INT_P (offset_rtx))
    stack_parm = gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
  else if (offset_rtx != const0_rtx)
    stack_parm = plus_constant (Pmode, stack_parm, INTVAL (offset_rtx));
  stack_parm = gen_rtx_MEM (data->promoted_mode, stack_parm);
and deal specially with GET_CODE (crtl->args.internal_arg_pointer)
in var-tracking.c.
rs6000/powerpcspe with -fsplit-stack are the only cases where
crtl->args.internal_arg_pointer is not a REG, so just running libgo
testsuite on powerpc{,64,64le} should cover it all.

	Jakub
Aaron Sawdey Jan. 30, 2018, 12:50 p.m. UTC | #9
On Tue, 2018-01-30 at 11:13 +0100, Jakub Jelinek wrote:
> On Tue, Jan 30, 2018 at 03:55:58AM -0600, Segher Boessenkool wrote:
> > > But in that case, what does the copying?
> > 
> > I don't know.  Aaron will look at it, but timezones etc. :-)

Indeed I did see unshare_all_rtl() copying internal_arg_pointer. But
also several places in function.c:

assign_parm_adjust_entry_rtl:
	  move_block_from_reg (REGNO (entry_parm),
			       validize_mem (copy_rtx (stack_parm)),
			       data->partial / UNITS_PER_WORD);

assign_parm_setup_reg:
  /* Copy the value into the register, thus bridging between
     assign_parm_find_data_types and expand_expr_real_1.  */

  equiv_stack_parm = data->stack_parm;
  validated_mem = validize_mem (copy_rtx (data->entry_parm));

assign_parm_setup_block:
      mem = validize_mem (copy_rtx (stack_parm));

in expr.c:

expand_expr_real_1:
      /* DECL_MODE might change when TYPE_MODE depends on attribute target
	 settings for VECTOR_TYPE_P that might switch for the function.  */
      if (currently_expanding_to_rtl
	  && code == VAR_DECL && MEM_P (decl_rtl)
	  && VECTOR_TYPE_P (type) && exp && DECL_MODE (exp) != mode)
	decl_rtl = change_address (decl_rtl, TYPE_MODE (type), 0);
      else
	decl_rtl = copy_rtx (decl_rtl);


> > 
> > > That's what seems strange.  I can see why we'd have two nested
> > > pluses with the inner plus being pointer-equal to
> > > internal_arg_ptr.
> > > And I can see why we'd have a single canonical plus (which IMO
> > > would
> > > be better, but I agree it's not stage 4 material).  It's having
> > > the two
> > > nested pluses without maintaining pointer equality that seems
> > > strange.
> > 
> > The inner plus is *not* pointer-equal, that is the
> > problem.  Something
> > did copy_rtx (or such) on it, many things do.  We can tell you what
> > exactly later today.
> 
> Most likely unshare_all_rtl, which does:
>   for (tree decl = DECL_ARGUMENTS (cfun->decl); decl; decl =
> DECL_CHAIN (decl))
>     {
>       if (DECL_RTL_SET_P (decl))
>         SET_DECL_RTL (decl, copy_rtx_if_shared (DECL_RTL (decl)));
>       DECL_INCOMING_RTL (decl) = copy_rtx_if_shared
> (DECL_INCOMING_RTL (decl));
>     }
> 
> Anyway, my preference would be to change that gen_rtx_PLUS into
>   stack_parm = crtl->args.internal_arg_pointer;
>   if (!CONST_INT_P (offset_rtx))
>     stack_parm = gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
>   else if (offset_rtx != const0_rtx)
>     stack_parm = plus_constant (Pmode, stack_parm, INTVAL
> (offset_rtx));
>   stack_parm = gen_rtx_MEM (data->promoted_mode, stack_parm);
> and deal specially with GET_CODE (crtl->args.internal_arg_pointer)
> in var-tracking.c.
> rs6000/powerpcspe with -fsplit-stack are the only cases where
> crtl->args.internal_arg_pointer is not a REG, so just running libgo
> testsuite on powerpc{,64,64le} should cover it all.

I'll give this a try today when I get to the office.

Thanks,
    Aaron


> 
> 	Jakub
>
Jakub Jelinek Jan. 30, 2018, 1:04 p.m. UTC | #10
On Tue, Jan 30, 2018 at 06:50:50AM -0600, Aaron Sawdey wrote:
> > Anyway, my preference would be to change that gen_rtx_PLUS into
> >   stack_parm = crtl->args.internal_arg_pointer;
> >   if (!CONST_INT_P (offset_rtx))
> >     stack_parm = gen_rtx_PLUS (Pmode, stack_parm, offset_rtx);
> >   else if (offset_rtx != const0_rtx)
> >     stack_parm = plus_constant (Pmode, stack_parm, INTVAL
> > (offset_rtx));
> >   stack_parm = gen_rtx_MEM (data->promoted_mode, stack_parm);
> > and deal specially with GET_CODE (crtl->args.internal_arg_pointer)
> > in var-tracking.c.
> > rs6000/powerpcspe with -fsplit-stack are the only cases where
> > crtl->args.internal_arg_pointer is not a REG, so just running libgo
> > testsuite on powerpc{,64,64le} should cover it all.
> 
> I'll give this a try today when I get to the office.

On IRC when discussing it with Segher this morning we've come to the
conclusion that it would be best if rs6000 just followed what all other
ports to, i.e. return a pseudo from the target hook, like:

--- gcc/config/rs6000/rs6000.c	2018-01-30 12:30:27.416360076 +0100
+++ gcc/config/rs6000/rs6000.c	2018-01-30 13:59:07.360639803 +0100
@@ -29602,8 +29602,9 @@ rs6000_internal_arg_pointer (void)
 	  emit_insn_after (pat, get_insns ());
 	  pop_topmost_sequence ();
 	}
-      return plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
-			    FIRST_PARM_OFFSET (current_function_decl));
+      rtx ret = plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
+			       FIRST_PARM_OFFSET (current_function_decl));
+      return copy_to_reg (ret);
     }
   return virtual_incoming_args_rtx;
 }

copy_to_reg is what e.g. the generic or pa target hook conditionally uses.

Optionally, check also whether the
          push_topmost_sequence ();
          emit_insn_after (pat, get_insns ());
          pop_topmost_sequence ();
stuff is needed instead of just
	emit_insn (pat);
and whether it needs to be guarded with cfun->machine->split_stack_arg_pointer
== NULL, because the target hook is called exactly once for each function.

	Jakub
Aaron Sawdey Jan. 30, 2018, 4:21 p.m. UTC | #11
On Tue, 2018-01-30 at 14:04 +0100, Jakub Jelinek wrote:
> On IRC when discussing it with Segher this morning we've come to the
> conclusion that it would be best if rs6000 just followed what all
> other
> ports to, i.e. return a pseudo from the target hook, like:
> 
> --- gcc/config/rs6000/rs6000.c	2018-01-30 12:30:27.416360076
> +0100
> +++ gcc/config/rs6000/rs6000.c	2018-01-30 13:59:07.360639803
> +0100
> @@ -29602,8 +29602,9 @@ rs6000_internal_arg_pointer (void)
>  	  emit_insn_after (pat, get_insns ());
>  	  pop_topmost_sequence ();
>  	}
> -      return plus_constant (Pmode, cfun->machine-
> >split_stack_arg_pointer,
> -			    FIRST_PARM_OFFSET
> (current_function_decl));
> +      rtx ret = plus_constant (Pmode, cfun->machine-
> >split_stack_arg_pointer,
> +			       FIRST_PARM_OFFSET
> (current_function_decl));
> +      return copy_to_reg (ret);
>      }
>    return virtual_incoming_args_rtx;
>  }
> 
> copy_to_reg is what e.g. the generic or pa target hook conditionally
> uses.

This fix looks good, passes bootstrap, go tests run. 

Segher is currently regtesting on ppc64le power9. OK for trunk if tests
pass?

2018-01-30  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>

	* config/rs6000/rs6000.c (rs6000_internal_arg_pointer ): Only return
	a reg rtx.
Jakub Jelinek Jan. 30, 2018, 4:24 p.m. UTC | #12
On Tue, Jan 30, 2018 at 10:21:33AM -0600, Aaron Sawdey wrote:
> 2018-01-30  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_internal_arg_pointer ): Only return

No space before ): .  Will defer ack to Segher or David.

> 	a reg rtx.

> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 257188)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -29602,8 +29602,9 @@
>  	  emit_insn_after (pat, get_insns ());
>  	  pop_topmost_sequence ();
>  	}
> -      return plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
> -			    FIRST_PARM_OFFSET (current_function_decl));
> +      rtx ret = plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
> +			       FIRST_PARM_OFFSET (current_function_decl));
> +      return copy_to_reg (ret);
>      }
>    return virtual_incoming_args_rtx;
>  }


	Jakub
Segher Boessenkool Jan. 30, 2018, 5:13 p.m. UTC | #13
Hi!

On Tue, Jan 30, 2018 at 10:21:33AM -0600, Aaron Sawdey wrote:
> This fix looks good, passes bootstrap, go tests run. 
> 
> Segher is currently regtesting on ppc64le power9. OK for trunk if tests
> pass?
> 
> 2018-01-30  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_internal_arg_pointer ): Only return
> 	a reg rtx.

Yes please, with the changelog typo fixed as Jakub said.

Thanks!  Glad this is over with :-)

I think it needs backports, too?  Those are okay as well.


Segher


> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 257188)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -29602,8 +29602,9 @@
>  	  emit_insn_after (pat, get_insns ());
>  	  pop_topmost_sequence ();
>  	}
> -      return plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
> -			    FIRST_PARM_OFFSET (current_function_decl));
> +      rtx ret = plus_constant (Pmode, cfun->machine->split_stack_arg_pointer,
> +			       FIRST_PARM_OFFSET (current_function_decl));
> +      return copy_to_reg (ret);
>      }
>    return virtual_incoming_args_rtx;
>  }

Patch
diff mbox series

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c  (revision 257159)
+++ gcc/var-tracking.c  (working copy)
@@ -9668,10 +9668,10 @@ 
      into MEMs based on the argument pointer, so that incoming doesn't
      depend on a pseudo.  */
   if (MEM_P (incoming)
-      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
+      && (rtx_equal_p (XEXP (incoming, 0), crtl-
>args.internal_arg_pointer)
          || (GET_CODE (XEXP (incoming, 0)) == PLUS
-             && XEXP (XEXP (incoming, 0), 0)
-                == crtl->args.internal_arg_pointer
+             && rtx_equal_p (XEXP (XEXP (incoming, 0), 0),
+                             crtl->args.internal_arg_pointer)
              && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
     {