[PATCH][PR debug/83758] look more carefully for internal_arg_pointer in vt_add_function_parameter()

[Richard Sandiford]

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