| 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() | expand |
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
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
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
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
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
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
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
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
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 >
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
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.
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
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; > }
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))))) {
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);