diff mbox

Fix PR64876, regressions in powerpc64 Go testsuite

Message ID 20150205102901.GU14796@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Feb. 5, 2015, 10:29 a.m. UTC
On Thu, Feb 05, 2015 at 08:12:25AM +0100, Jakub Jelinek wrote:
> On Thu, Feb 05, 2015 at 02:09:54PM +1030, Alan Modra wrote:
> > Jakub, was your suggestion to use get_last_insn_anywhere() based on
> > not wanting to expose details that should be internal to
> > emit-rtl.[ch]?
> 
> Yes.  But if it doesn't work for what you want, either add a new accessor or
> use it directly.

Thanks, I'll use it directly now and have a patch in the works to tidy
m32c.c and rs6000.c.

David, here is the revised patch.  Bootstrapped etc. powerpc64-linux,
and fixes a few more Go testsuite failures compared to the last one..

	PR target/64876
	* config/rs6000/rs6000.c (chain_already_loaded): New function.
	(rs6000_call_aix): Use it.

Comments

Jakub Jelinek Feb. 5, 2015, 10:31 a.m. UTC | #1
On Thu, Feb 05, 2015 at 08:59:01PM +1030, Alan Modra wrote:
> On Thu, Feb 05, 2015 at 08:12:25AM +0100, Jakub Jelinek wrote:
> > On Thu, Feb 05, 2015 at 02:09:54PM +1030, Alan Modra wrote:
> > > Jakub, was your suggestion to use get_last_insn_anywhere() based on
> > > not wanting to expose details that should be internal to
> > > emit-rtl.[ch]?
> > 
> > Yes.  But if it doesn't work for what you want, either add a new accessor or
> > use it directly.
> 
> Thanks, I'll use it directly now and have a patch in the works to tidy
> m32c.c and rs6000.c.
> 
> David, here is the revised patch.  Bootstrapped etc. powerpc64-linux,
> and fixes a few more Go testsuite failures compared to the last one..
> 
> 	PR target/64876
> 	* config/rs6000/rs6000.c (chain_already_loaded): New function.
> 	(rs6000_call_aix): Use it.
> 
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 220433)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -32919,7 +32919,29 @@ rs6000_legitimate_constant_p (machine_mode mode, r
>  }
>  
>  
> +/* Return TRUE iff the sequence ending in LAST sets the static chain.  */
>  
> +static bool
> +chain_already_loaded (rtx_insn *last)
> +{
> +  for (; last != NULL; last = PREV_INSN (last))
> +    {
> +      if (NONJUMP_INSN_P (last))
> +	{
> +	  rtx patt = PATTERN (last);
> +
> +	  if (GET_CODE (patt) == SET)
> +	    {
> +	      rtx lhs = XEXP (patt, 0);
> +
> +	      if (REG_P (lhs) && REGNO (lhs) == STATIC_CHAIN_REGNUM)
> +		return true;
> +	    }

Shouldn't you stop at CALL_INSNs?  I mean in that case it would load the
static chain for the other call and not the current one.
Or do you have some guarantee that is in yet another sequence?

	Jakub
Alan Modra Feb. 5, 2015, 11:47 a.m. UTC | #2
On Thu, Feb 05, 2015 at 11:31:53AM +0100, Jakub Jelinek wrote:
> On Thu, Feb 05, 2015 at 08:59:01PM +1030, Alan Modra wrote:
> > On Thu, Feb 05, 2015 at 08:12:25AM +0100, Jakub Jelinek wrote:
> > > On Thu, Feb 05, 2015 at 02:09:54PM +1030, Alan Modra wrote:
> > > > Jakub, was your suggestion to use get_last_insn_anywhere() based on
> > > > not wanting to expose details that should be internal to
> > > > emit-rtl.[ch]?
> > > 
> > > Yes.  But if it doesn't work for what you want, either add a new accessor or
> > > use it directly.
> > 
> > Thanks, I'll use it directly now and have a patch in the works to tidy
> > m32c.c and rs6000.c.
> > 
> > David, here is the revised patch.  Bootstrapped etc. powerpc64-linux,
> > and fixes a few more Go testsuite failures compared to the last one..
> > 
> > 	PR target/64876
> > 	* config/rs6000/rs6000.c (chain_already_loaded): New function.
> > 	(rs6000_call_aix): Use it.
> > 
> > Index: gcc/config/rs6000/rs6000.c
> > ===================================================================
> > --- gcc/config/rs6000/rs6000.c	(revision 220433)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -32919,7 +32919,29 @@ rs6000_legitimate_constant_p (machine_mode mode, r
> >  }
> >  
> >  
> > +/* Return TRUE iff the sequence ending in LAST sets the static chain.  */
> >  
> > +static bool
> > +chain_already_loaded (rtx_insn *last)
> > +{
> > +  for (; last != NULL; last = PREV_INSN (last))
> > +    {
> > +      if (NONJUMP_INSN_P (last))
> > +	{
> > +	  rtx patt = PATTERN (last);
> > +
> > +	  if (GET_CODE (patt) == SET)
> > +	    {
> > +	      rtx lhs = XEXP (patt, 0);
> > +
> > +	      if (REG_P (lhs) && REGNO (lhs) == STATIC_CHAIN_REGNUM)
> > +		return true;
> > +	    }
> 
> Shouldn't you stop at CALL_INSNs?  I mean in that case it would load the
> static chain for the other call and not the current one.
> Or do you have some guarantee that is in yet another sequence?

Yes, it is in another sequence.

rs6000_aix_call is guaranteed to be inside a GEN_CALL or GEN_CALL_VALUE,
so chain_already_loaded is guaranteed to be looking at rtl generated by
calls.c:emit_call_1.  emit_call_1 is called from expand_call and
emit_library_call{,value} via emit_library_call_value_1.  So far as I
know, we won't be calling library functions via an indirect call, so
chain_already_loaded will only be hit for the expand_call case.
expand_call uses start_sequence, which means we have rtl for the call
nicely isolated from other call rtl (except libcalls maybe, not sure
if that still happens).
David Edelsohn Feb. 5, 2015, 3 p.m. UTC | #3
On Thu, Feb 5, 2015 at 5:29 AM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Feb 05, 2015 at 08:12:25AM +0100, Jakub Jelinek wrote:
>> On Thu, Feb 05, 2015 at 02:09:54PM +1030, Alan Modra wrote:
>> > Jakub, was your suggestion to use get_last_insn_anywhere() based on
>> > not wanting to expose details that should be internal to
>> > emit-rtl.[ch]?
>>
>> Yes.  But if it doesn't work for what you want, either add a new accessor or
>> use it directly.
>
> Thanks, I'll use it directly now and have a patch in the works to tidy
> m32c.c and rs6000.c.
>
> David, here is the revised patch.  Bootstrapped etc. powerpc64-linux,
> and fixes a few more Go testsuite failures compared to the last one..
>
>         PR target/64876
>         * config/rs6000/rs6000.c (chain_already_loaded): New function.
>         (rs6000_call_aix): Use it.

Okay.

Thanks for solving this!

- David
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 220433)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -32919,7 +32919,29 @@  rs6000_legitimate_constant_p (machine_mode mode, r
 }
 
 
+/* Return TRUE iff the sequence ending in LAST sets the static chain.  */
 
+static bool
+chain_already_loaded (rtx_insn *last)
+{
+  for (; last != NULL; last = PREV_INSN (last))
+    {
+      if (NONJUMP_INSN_P (last))
+	{
+	  rtx patt = PATTERN (last);
+
+	  if (GET_CODE (patt) == SET)
+	    {
+	      rtx lhs = XEXP (patt, 0);
+
+	      if (REG_P (lhs) && REGNO (lhs) == STATIC_CHAIN_REGNUM)
+		return true;
+	    }
+	}
+    }
+  return false;
+}
+
 /* Expand code to perform a call under the AIX or ELFv2 ABI.  */
 
 void
@@ -33002,7 +33024,9 @@  rs6000_call_aix (rtx value, rtx func_desc, rtx fla
 	     originally direct, the 3rd word has not been written since no
 	     trampoline has been built, so we ought not to load it, lest we
 	     override a static chain value.  */
-	  if (!direct_call_p && TARGET_POINTERS_TO_NESTED_FUNCTIONS)
+	  if (!direct_call_p
+	      && TARGET_POINTERS_TO_NESTED_FUNCTIONS
+	      && !chain_already_loaded (crtl->emit.sequence_stack->last))
 	    {
 	      rtx sc_reg = gen_rtx_REG (Pmode, STATIC_CHAIN_REGNUM);
 	      rtx func_sc_offset = GEN_INT (2 * GET_MODE_SIZE (Pmode));