diff mbox series

PR97107, libgo fails to build for power10

Message ID 20200922002512.GA5452@bubble.grove.modra.org
State New
Headers show
Series PR97107, libgo fails to build for power10 | expand

Commit Message

Alan Modra Sept. 22, 2020, 12:25 a.m. UTC
Calls from split-stack code to non-split-stack code need to expand
mapped stack memory via __morestack.  Even tail calls.

__morestack is quite a surprising function on powerpc in that it calls
back to its caller, and a tail call will continue running in the
context of extra mapped stack.

Bootstrapped and regression tested on power8, and built for power10.

	PR target/97107
	* config/rs6000/rs6000-internal.h (struct rs6000_stack): Improve
	calls_p comment.
	* config/rs6000/rs6000-logue.c (rs6000_stack_info): Likewise.
	(rs6000_expand_split_stack_prologue): Emit the prologue for
	functions that make a sibling call.

Comments

Segher Boessenkool Sept. 22, 2020, 11:59 p.m. UTC | #1
Hi!

On Tue, Sep 22, 2020 at 09:55:12AM +0930, Alan Modra wrote:
> Calls from split-stack code to non-split-stack code need to expand
> mapped stack memory via __morestack.  Even tail calls.
> 
> __morestack is quite a surprising function on powerpc in that it calls
> back to its caller, and a tail call will continue running in the
> context of extra mapped stack.

Also known as "pure evil" :-)

> 	PR target/97107
> 	* config/rs6000/rs6000-internal.h (struct rs6000_stack): Improve
> 	calls_p comment.
> 	* config/rs6000/rs6000-logue.c (rs6000_stack_info): Likewise.
> 	(rs6000_expand_split_stack_prologue): Emit the prologue for
> 	functions that make a sibling call.

>    if (!info->push_p)
> -    return;
> +    {
> +      /* We need the -fsplit-stack prologue for functions that make
> +	 tail calls.  Tail calls don't count against crtl->is_leaf.  */
> +      for (insn = get_topmost_sequence ()->first; insn; insn = NEXT_INSN (insn))
> +	if (CALL_P (insn))
> +	  break;
> +      if (!insn)
> +	return;
> +    }

I don't think that get_topmost_sequence is correct.

Other than that this is fine for trunk (and backports).  Thanks!


Segher
Alan Modra Sept. 23, 2020, 12:30 a.m. UTC | #2
Hi Segher,

On Tue, Sep 22, 2020 at 06:59:42PM -0500, Segher Boessenkool wrote:
> On Tue, Sep 22, 2020 at 09:55:12AM +0930, Alan Modra wrote:
> >    if (!info->push_p)
> > -    return;
> > +    {
> > +      /* We need the -fsplit-stack prologue for functions that make
> > +	 tail calls.  Tail calls don't count against crtl->is_leaf.  */
> > +      for (insn = get_topmost_sequence ()->first; insn; insn = NEXT_INSN (insn))
> > +	if (CALL_P (insn))
> > +	  break;
> > +      if (!insn)
> > +	return;
> > +    }
> 
> I don't think that get_topmost_sequence is correct.

We get here from inside a sequence so we definitely need a way to look
at the function RTL rather than the empty sequence.  You want
push_topmost_sequence / pop_topmost_sequence around the rtl scan?

I wasn't aware there was a need for that when not emitting insns.  And
can't see any reason why when I look carefully, except that seems to
be customary, grep only shows one other place using
get_topmost_sequence as I did.

> Other than that this is fine for trunk (and backports).  Thanks!
> 
> 
> Segher
Segher Boessenkool Sept. 23, 2020, 12:52 a.m. UTC | #3
On Wed, Sep 23, 2020 at 10:00:01AM +0930, Alan Modra wrote:
> Hi Segher,
> 
> On Tue, Sep 22, 2020 at 06:59:42PM -0500, Segher Boessenkool wrote:
> > On Tue, Sep 22, 2020 at 09:55:12AM +0930, Alan Modra wrote:
> > >    if (!info->push_p)
> > > -    return;
> > > +    {
> > > +      /* We need the -fsplit-stack prologue for functions that make
> > > +	 tail calls.  Tail calls don't count against crtl->is_leaf.  */
> > > +      for (insn = get_topmost_sequence ()->first; insn; insn = NEXT_INSN (insn))
> > > +	if (CALL_P (insn))
> > > +	  break;
> > > +      if (!insn)
> > > +	return;
> > > +    }
> > 
> > I don't think that get_topmost_sequence is correct.
> 
> We get here from inside a sequence so we definitely need a way to look
> at the function RTL rather than the empty sequence.  You want
> push_topmost_sequence / pop_topmost_sequence around the rtl scan?

That will not help I think.

It isn't obvious that the outer sequence is always what we want.  If
there is some nicer way to get at the info you want, that would help.

But, this is the expander only -- so I guess we are okay here, it is
much limited when we can be called here.  Add a comment though?

> I wasn't aware there was a need for that when not emitting insns.  And
> can't see any reason why when I look carefully, except that seems to
> be customary, grep only shows one other place using
> get_topmost_sequence as I did.

Yes, and not in a widely used target.  So I wonder if there is some less
tricky way to get at the wanted info?


Segher
Alan Modra Sept. 24, 2020, 3:42 a.m. UTC | #4
On Tue, Sep 22, 2020 at 07:52:53PM -0500, Segher Boessenkool wrote:
> It isn't obvious that the outer sequence is always what we want.  If
> there is some nicer way to get at the info you want, that would help.
> 
> But, this is the expander only -- so I guess we are okay here, it is
> much limited when we can be called here.  Add a comment though?

Pushed with a comment.  Thanks!
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000-internal.h b/gcc/config/rs6000/rs6000-internal.h
index 9caef013a71..32681b6cece 100644
--- a/gcc/config/rs6000/rs6000-internal.h
+++ b/gcc/config/rs6000/rs6000-internal.h
@@ -32,7 +32,7 @@  typedef struct rs6000_stack {
   int cr_save_p;		/* true if the CR reg needs to be saved */
   unsigned int vrsave_mask;	/* mask of vec registers to save */
   int push_p;			/* true if we need to allocate stack space */
-  int calls_p;			/* true if the function makes any calls */
+  int calls_p;			/* true if there are non-sibling calls */
   int world_save_p;		/* true if we're saving *everything*:
 				   r13-r31, cr, f14-f31, vrsave, v20-v31  */
   enum rs6000_abi abi;		/* which ABI to use */
diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index 0f88ec19929..cb08c25c795 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -714,7 +714,7 @@  rs6000_stack_info (void)
   info->altivec_size = 16 * (LAST_ALTIVEC_REGNO + 1
 				 - info->first_altivec_reg_save);
 
-  /* Does this function call anything?  */
+  /* Does this function call anything (apart from sibling calls)?  */
   info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame);
 
   /* Determine if we need to save the condition code registers.  */
@@ -5479,7 +5479,15 @@  rs6000_expand_split_stack_prologue (void)
   gcc_assert (flag_split_stack && reload_completed);
 
   if (!info->push_p)
-    return;
+    {
+      /* We need the -fsplit-stack prologue for functions that make
+	 tail calls.  Tail calls don't count against crtl->is_leaf.  */
+      for (insn = get_topmost_sequence ()->first; insn; insn = NEXT_INSN (insn))
+	if (CALL_P (insn))
+	  break;
+      if (!insn)
+	return;
+    }
 
   if (global_regs[29])
     {