| Message ID | 20200922002512.GA5452@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | PR97107, libgo fails to build for power10 | expand |
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
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
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
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 --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]) {