| Message ID | 20210603065408.47912-1-hongtao.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] CALL_INSN may not be a real function call. | expand |
Ping, This is a splitted middle-end patch as a follow up of https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571544.html On Thu, Jun 3, 2021 at 2:54 PM liuhongt via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > fake call, it won't have its own function stack. > > gcc/ChangeLog > > PR target/82735 > * df-scan.c (df_get_call_refs): When call_insn is a fake call, > it won't use stack pointer reg. > * final.c (leaf_function_p): When call_insn is a fake call, it > won't affect caller as a leaf function. > * reg-stack.c (callee_clobbers_any_stack_reg): New. > (subst_stack_regs): When call_insn doesn't clobber any stack > reg, don't clear the arguments. > * rtl.c (shallow_copy_rtx): Don't clear flag used when orig is > a insn. > * shrink-wrap.c (requires_stack_frame_p): No need for stack > frame for a fake call. > * rtl.h (FAKE_CALL_P): New macro. > --- > gcc/df-scan.c | 3 ++- > gcc/final.c | 3 ++- > gcc/reg-stack.c | 18 +++++++++++++++++- > gcc/rtl.c | 6 ++++-- > gcc/rtl.h | 5 +++++ > gcc/shrink-wrap.c | 2 +- > 6 files changed, 31 insertions(+), 6 deletions(-) > > diff --git a/gcc/df-scan.c b/gcc/df-scan.c > index 6691c3e8357..1268536b3f0 100644 > --- a/gcc/df-scan.c > +++ b/gcc/df-scan.c > @@ -3090,7 +3090,8 @@ df_get_call_refs (class df_collection_rec *collection_rec, > > for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) > { > - if (i == STACK_POINTER_REGNUM) > + if (i == STACK_POINTER_REGNUM > + && !FAKE_CALL_P (insn_info->insn)) > /* The stack ptr is used (honorarily) by a CALL insn. */ > df_ref_record (DF_REF_BASE, collection_rec, regno_reg_rtx[i], > NULL, bb, insn_info, DF_REF_REG_USE, > diff --git a/gcc/final.c b/gcc/final.c > index e0a70fcd830..817f7722cb2 100644 > --- a/gcc/final.c > +++ b/gcc/final.c > @@ -4109,7 +4109,8 @@ leaf_function_p (void) > for (insn = get_insns (); insn; insn = NEXT_INSN (insn)) > { > if (CALL_P (insn) > - && ! SIBLING_CALL_P (insn)) > + && ! SIBLING_CALL_P (insn) > + && ! FAKE_CALL_P (insn)) > return 0; > if (NONJUMP_INSN_P (insn) > && GET_CODE (PATTERN (insn)) == SEQUENCE > diff --git a/gcc/reg-stack.c b/gcc/reg-stack.c > index 25210f0c17f..1d9ea035cf4 100644 > --- a/gcc/reg-stack.c > +++ b/gcc/reg-stack.c > @@ -174,6 +174,7 @@ > #include "reload.h" > #include "tree-pass.h" > #include "rtl-iter.h" > +#include "function-abi.h" > > #ifdef STACK_REGS > > @@ -2368,6 +2369,18 @@ subst_asm_stack_regs (rtx_insn *insn, stack_ptr regstack) > } > } > } > + > +/* Return true if a function call is allowed to alter some or all bits > + of any stack reg. */ > +static bool > +callee_clobbers_any_stack_reg (const function_abi & callee_abi) > +{ > + for (unsigned regno = FIRST_STACK_REG; regno <= LAST_STACK_REG; regno++) > + if (callee_abi.clobbers_at_least_part_of_reg_p (regno)) > + return true; > + return false; > +} > + > > /* Substitute stack hard reg numbers for stack virtual registers in > INSN. Non-stack register numbers are not changed. REGSTACK is the > @@ -2382,7 +2395,10 @@ subst_stack_regs (rtx_insn *insn, stack_ptr regstack) > bool control_flow_insn_deleted = false; > int i; > > - if (CALL_P (insn)) > + /* If the target of the call doesn't clobber any stack registers, > + Don't clear the arguments. */ > + if (CALL_P (insn) > + && callee_clobbers_any_stack_reg (insn_callee_abi (insn))) > { > int top = regstack->top; > > diff --git a/gcc/rtl.c b/gcc/rtl.c > index b0ba1ff684c..aaee882f5ca 100644 > --- a/gcc/rtl.c > +++ b/gcc/rtl.c > @@ -395,8 +395,10 @@ shallow_copy_rtx (const_rtx orig MEM_STAT_DECL) > case SCRATCH: > break; > default: > - /* For all other RTXes clear the used flag on the copy. */ > - RTX_FLAG (copy, used) = 0; > + /* For all other RTXes clear the used flag on the copy. > + CALL_INSN use "used" flag to indicate it's a fake call. */ > + if (!INSN_P (orig)) > + RTX_FLAG (copy, used) = 0; > break; > } > return copy; > diff --git a/gcc/rtl.h b/gcc/rtl.h > index 35178b5bfac..5ed0d6dd6fa 100644 > --- a/gcc/rtl.h > +++ b/gcc/rtl.h > @@ -839,6 +839,11 @@ struct GTY(()) rtvec_def { > /* Predicate yielding nonzero iff X is a call insn. */ > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > +/* 1 if RTX is a call_insn for a fake call. > + CALL_INSN use "used" flag to indicate it's a fake call. */ > +#define FAKE_CALL_P(RTX) \ > + (RTL_FLAG_CHECK1 ("FAKE_CALL_P", (RTX), CALL_INSN)->used) > + > /* Predicate yielding nonzero iff X is an insn that cannot jump. */ > #define NONJUMP_INSN_P(X) (GET_CODE (X) == INSN) > > diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c > index ba7b5cd56fd..5e60f34f749 100644 > --- a/gcc/shrink-wrap.c > +++ b/gcc/shrink-wrap.c > @@ -57,7 +57,7 @@ requires_stack_frame_p (rtx_insn *insn, HARD_REG_SET prologue_used, > HARD_REG_SET hardregs; > unsigned regno; > > - if (CALL_P (insn)) > + if (CALL_P (insn) && !FAKE_CALL_P (insn)) > return !SIBLING_CALL_P (insn); > > /* We need a frame to get the unique CFA expected by the unwinder. */ > -- > 2.18.1 >
On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt wrote: > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > fake call, it won't have its own function stack. > > gcc/ChangeLog > > PR target/82735 > * df-scan.c (df_get_call_refs): When call_insn is a fake call, > it won't use stack pointer reg. > * final.c (leaf_function_p): When call_insn is a fake call, it > won't affect caller as a leaf function. > * reg-stack.c (callee_clobbers_any_stack_reg): New. > (subst_stack_regs): When call_insn doesn't clobber any stack > reg, don't clear the arguments. > * rtl.c (shallow_copy_rtx): Don't clear flag used when orig is > a insn. > * shrink-wrap.c (requires_stack_frame_p): No need for stack > frame for a fake call. > * rtl.h (FAKE_CALL_P): New macro. Ok, thanks. Jakub
Hi! I ran into this in shrink-wrap.c today. On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > fake call, it won't have its own function stack. Could you document somewhere what a "fake call" *is*? Including what that means to RTL, how this is expected to be used, etc.? In rtl.h is fine with me, but as it is, no one can know when to use this. What does "its own function stack" mean in the description here? You can only put FAKE_CALL on functions that do not have a stack frame? But that is never true on x86, so that cannot be it, unless there isn't a call instruction at all? But then, why use an RTL call insn for this? Other targets simply do not use an RTL "call" when they want to hide such an instruction, why can't you do that here, wouldn't that work much better? There are many more insns that you may want to hide. The traditional solution is to use unspecs, which very directly hides all details. Segher
On 7/5/2021 5:30 PM, Segher Boessenkool wrote: > Hi! > > I ran into this in shrink-wrap.c today. > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: >> Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a >> fake call, it won't have its own function stack. > Could you document somewhere what a "fake call" *is*? Including what > that means to RTL, how this is expected to be used, etc.? In rtl.h is > fine with me, but as it is, no one can know when to use this. What does > "its own function stack" mean in the description here? You can only put > FAKE_CALL on functions that do not have a stack frame? But that is > never true on x86, so that cannot be it, unless there isn't a call > instruction at all? But then, why use an RTL call insn for this? > > Other targets simply do not use an RTL "call" when they want to hide > such an instruction, why can't you do that here, wouldn't that work much > better? There are many more insns that you may want to hide. The > traditional solution is to use unspecs, which very directly hides all > details. It reminds me a bit of millicode calls on the PA or calls to special routines in libgcc. They're calls to functions, but those functions do not follow the standard ABI. I'd like to remove INSN_REFERENCES_ARE_DELAYED and instead use the new fake call mechanism, but I haven't tried it or even looked at the fake call bits enough to know if that's possible. jeff
On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > I ran into this in shrink-wrap.c today. > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > fake call, it won't have its own function stack. > > Could you document somewhere what a "fake call" *is*? Including what > that means to RTL, how this is expected to be used, etc.? In rtl.h is fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for #define FAKE_CALL_P(RTX) in rtl.h > fine with me, but as it is, no one can know when to use this. What does > "its own function stack" mean in the description here? You can only put > FAKE_CALL on functions that do not have a stack frame? But that is > never true on x86, so that cannot be it, unless there isn't a call > instruction at all? But then, why use an RTL call insn for this? > > Other targets simply do not use an RTL "call" when they want to hide > such an instruction, why can't you do that here, wouldn't that work much > better? There are many more insns that you may want to hide. The > traditional solution is to use unspecs, which very directly hides all > details. It's explained here, > >> Yeah. Initially clobber_high seemed like the best appraoch for > >> handling the tlsdesc thing, but in practice it was too difficult > >> to shoe-horn the concept in after the fact, when so much rtl > >> infrastructure wasn't prepared to deal with it. The old support > >> didn't handle all cases and passes correctly, and handled others > >> suboptimally. > >> > >> I think it would be worth using the same approach as > >> https://gcc.gnu.org/legacy-ml/gcc-patches/2019-09/msg01466.html for > >> vzeroupper: represent the instructions as call_insns in which the > >> call has a special vzeroupper ABI. I think that's likely to lead > >> to better code than clobber_high would (or at least, it did for tlsdesc). refer to [1] for more details [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570265.html > > > Segher
On Tue, Jul 6, 2021 at 8:03 AM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 7/5/2021 5:30 PM, Segher Boessenkool wrote: > > Hi! > > > > I ran into this in shrink-wrap.c today. > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > >> Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > >> fake call, it won't have its own function stack. > > Could you document somewhere what a "fake call" *is*? Including what > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > fine with me, but as it is, no one can know when to use this. What does > > "its own function stack" mean in the description here? You can only put > > FAKE_CALL on functions that do not have a stack frame? But that is > > never true on x86, so that cannot be it, unless there isn't a call > > instruction at all? But then, why use an RTL call insn for this? > > > > Other targets simply do not use an RTL "call" when they want to hide > > such an instruction, why can't you do that here, wouldn't that work much > > better? There are many more insns that you may want to hide. The > > traditional solution is to use unspecs, which very directly hides all > > details. > It reminds me a bit of millicode calls on the PA or calls to special > routines in libgcc. They're calls to functions, but those functions do > not follow the standard ABI. I'd like to remove > INSN_REFERENCES_ARE_DELAYED and instead use the new fake call mechanism, > but I haven't tried it or even looked at the fake call bits enough to > know if that's possible. Fake call is used for TARGET_INSN_CALLEE_ABI which is used for vzeroupper in i386. vzeroupper clobber high part of ymm registers but leave low part unchanged, define it and call_insn with special callee ABI so that RA/CSE knows this instruction kills high parts of ymm registers, and can still optimize with lowpart. I didn't handle FAKE_CALL_P thoroughly in the RTL, but only changed the necessary parts so that I could get my patch to survive the regression test(also fix some optimization issues I observed). n through the tests> > jeff
On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > Hi! > > > > I ran into this in shrink-wrap.c today. > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > fake call, it won't have its own function stack. > > > > Could you document somewhere what a "fake call" *is*? Including what > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > #define FAKE_CALL_P(RTX) in rtl.h Here's the patch I'm going to check in. Document FAKE_CALL_P in comments. gcc/ChangeLog: * rtl.h (FAKE_CALL_P): Add comments for FAKE_CALL_P. diff --git a/gcc/rtl.h b/gcc/rtl.h index 5ed0d6dd6fa..9afc60f08d8 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { #define CALL_P(X) (GET_CODE (X) == CALL_INSN) /* 1 if RTX is a call_insn for a fake call. - CALL_INSN use "used" flag to indicate it's a fake call. */ + CALL_INSN use "used" flag to indicate it's a fake call. + Used by the x86 vzeroupper instruction, + in order to solve the problem of partial clobber registers, + vzeroupper is defined as a call_insn with a special callee_abi, + but it is not a real call and therefore has no function stack + of its own. + NB: FAKE_CALL_P is not handled thoroughly in the RTL. */ #define FAKE_CALL_P(RTX) \ (RTL_FLAG_CHECK1 ("FAKE_CALL_P", (RTX), CALL_INSN)->used)
On Wed, Jul 7, 2021 at 4:40 AM Hongtao Liu via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > > > > Hi! > > > > > > I ran into this in shrink-wrap.c today. > > > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > > fake call, it won't have its own function stack. > > > > > > Could you document somewhere what a "fake call" *is*? Including what > > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > > #define FAKE_CALL_P(RTX) in rtl.h > > > Here's the patch I'm going to check in. > > Document FAKE_CALL_P in comments. > > gcc/ChangeLog: > > * rtl.h (FAKE_CALL_P): Add comments for FAKE_CALL_P. > > diff --git a/gcc/rtl.h b/gcc/rtl.h > index 5ed0d6dd6fa..9afc60f08d8 100644 > --- a/gcc/rtl.h > +++ b/gcc/rtl.h > @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > /* 1 if RTX is a call_insn for a fake call. > - CALL_INSN use "used" flag to indicate it's a fake call. */ > + CALL_INSN use "used" flag to indicate it's a fake call. > + Used by the x86 vzeroupper instruction, > + in order to solve the problem of partial clobber registers, > + vzeroupper is defined as a call_insn with a special callee_abi, > + but it is not a real call and therefore has no function stack > + of its own. I think that's a big vague - you could then say a sibling or tail call to a function that doesn't set up a stack frame is fake as well? Maybe "CALL_INSN use "used" flag to indicate the instruction does not transfer control." thus that this call is not affecting regular control flow? (it might eventually still trap and thus cause non-call EH?) Not sure if "no function stack of its own" is a good constraint, vzeroupper does not perform any call or jump. > + NB: FAKE_CALL_P is not handled thoroughly in the RTL. */ > #define FAKE_CALL_P(RTX) \ > (RTL_FLAG_CHECK1 ("FAKE_CALL_P", (RTX), CALL_INSN)->used) > > > > > -- > BR, > Hongtao
Hi! On Wed, Jul 07, 2021 at 10:15:08AM +0200, Richard Biener wrote: > On Wed, Jul 7, 2021 at 4:40 AM Hongtao Liu via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > > > <segher@kernel.crashing.org> wrote: > > > > I ran into this in shrink-wrap.c today. > > > > > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > > > fake call, it won't have its own function stack. > > > > > > > > Could you document somewhere what a "fake call" *is*? Including what > > > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > > > #define FAKE_CALL_P(RTX) in rtl.h > > > > > > Here's the patch I'm going to check in. Which doesn't do any of the things I asked for :-( It doesn't say what a "fake call" is, it doesn't say what its semantics are, it doesn't say how it is exected to be used. So, a "FAKE_CALL" is very much a *real* call, on the RTL level, which is where we are here. But you want it to be treated differently because it will eventually be replaced by different insns. This causes all kinds of unrelated code to need confusing changes, made much worse because the name "FAKE_CALL" is the opposite of what it does. As long as your description of it only says how it is (ab)used in one case, I will call it a hack, and a gross hack at that. > > --- a/gcc/rtl.h > > +++ b/gcc/rtl.h > > @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { > > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > > > /* 1 if RTX is a call_insn for a fake call. > > - CALL_INSN use "used" flag to indicate it's a fake call. */ > > + CALL_INSN use "used" flag to indicate it's a fake call. > > + Used by the x86 vzeroupper instruction, > > + in order to solve the problem of partial clobber registers, > > + vzeroupper is defined as a call_insn with a special callee_abi, > > + but it is not a real call and therefore has no function stack > > + of its own. So because of this one thing (you need to insert partial clobbers) you force all kinds of unrelated code to have changes, namely, code thatt needs to do something with calls, but now you do not want to have that doone on some calls because you promise that call will disappear eventually, and it cannot cause any problems in the mean time? I am not convinced. This is not design, this is a terrible hack, this is the opposite direction we should go in. > that doesn't set up a stack frame is fake as well? Maybe > > "CALL_INSN use "used" flag to indicate the instruction > does not transfer control." > > thus that this call is not affecting regular control flow? (it might > eventually still trap and thus cause non-call EH?) How it is used in shrink-wrap requires it to not have a stack frame (in the compiler sense). > Not sure if "no function stack of its own" is a good constraint, > vzeroupper does not perform any call or jump. Yeah. This stuff needs a rethink. What is wrong with just using an unspec and clobbers? Segher
On Mon, Jul 05, 2021 at 06:03:21PM -0600, Jeff Law wrote: > It reminds me a bit of millicode calls on the PA or calls to special > routines in libgcc. They're calls to functions, but those functions do > not follow the standard ABI. Something with CALL_INSN_FUNCTION_USAGE? And maybe some clobbers? Segher
On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Wed, Jul 07, 2021 at 10:15:08AM +0200, Richard Biener wrote: > > On Wed, Jul 7, 2021 at 4:40 AM Hongtao Liu via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > > > > <segher@kernel.crashing.org> wrote: > > > > > I ran into this in shrink-wrap.c today. > > > > > > > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > > > > fake call, it won't have its own function stack. > > > > > > > > > > Could you document somewhere what a "fake call" *is*? Including what > > > > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > > > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > > > > #define FAKE_CALL_P(RTX) in rtl.h > > > > > > > > > Here's the patch I'm going to check in. > > Which doesn't do any of the things I asked for :-( It doesn't say what > a "fake call" is, it doesn't say what its semantics are, it doesn't say > how it is exected to be used. > > So, a "FAKE_CALL" is very much a *real* call, on the RTL level, which is > where we are here. But you want it to be treated differently because it > will eventually be replaced by different insns. > > This causes all kinds of unrelated code to need confusing changes, made > much worse because the name "FAKE_CALL" is the opposite of what it does. > > As long as your description of it only says how it is (ab)used in one > case, I will call it a hack, and a gross hack at that. > > > > > --- a/gcc/rtl.h > > > +++ b/gcc/rtl.h > > > @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { > > > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > > > > > /* 1 if RTX is a call_insn for a fake call. > > > - CALL_INSN use "used" flag to indicate it's a fake call. */ > > > + CALL_INSN use "used" flag to indicate it's a fake call. > > > + Used by the x86 vzeroupper instruction, > > > + in order to solve the problem of partial clobber registers, > > > + vzeroupper is defined as a call_insn with a special callee_abi, > > > + but it is not a real call and therefore has no function stack > > > + of its own. > > So because of this one thing (you need to insert partial clobbers) you > force all kinds of unrelated code to have changes, namely, code thatt > needs to do something with calls, but now you do not want to have that > doone on some calls because you promise that call will disappear > eventually, and it cannot cause any problems in the mean time? > > I am not convinced. This is not design, this is a terrible hack, this > is the opposite direction we should go in. > > > that doesn't set up a stack frame is fake as well? Maybe > > > > "CALL_INSN use "used" flag to indicate the instruction > > does not transfer control." > > > > thus that this call is not affecting regular control flow? (it might > > eventually still trap and thus cause non-call EH?) > > How it is used in shrink-wrap requires it to not have a stack frame (in > the compiler sense). > > > Not sure if "no function stack of its own" is a good constraint, > > vzeroupper does not perform any call or jump. > > Yeah. This stuff needs a rethink. > > What is wrong with just using an unspec and clobbers? > It's partial and **potential clobber**, if we add unspec and clobbers to the whole pack(8 or 16 xmm registers), it will force save/restore of registers that aren't really needed in the function, especially for 64bit MS ABI where lower 128bit are preserved across function calls. > > Segher
On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Wed, Jul 07, 2021 at 10:15:08AM +0200, Richard Biener wrote: > > On Wed, Jul 7, 2021 at 4:40 AM Hongtao Liu via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > > > > <segher@kernel.crashing.org> wrote: > > > > > I ran into this in shrink-wrap.c today. > > > > > > > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > > > > fake call, it won't have its own function stack. > > > > > > > > > > Could you document somewhere what a "fake call" *is*? Including what > > > > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > > > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > > > > #define FAKE_CALL_P(RTX) in rtl.h > > > > > > > > > Here's the patch I'm going to check in. > > Which doesn't do any of the things I asked for :-( It doesn't say what > a "fake call" is, it doesn't say what its semantics are, it doesn't say > how it is exected to be used. > > So, a "FAKE_CALL" is very much a *real* call, on the RTL level, which is > where we are here. But you want it to be treated differently because it > will eventually be replaced by different insns. It's CALL_INSN on the rtl level, but it's just a normal instruction that it doesn't have a call stack, and it doesn't affect the control flow > > This causes all kinds of unrelated code to need confusing changes, made > much worse because the name "FAKE_CALL" is the opposite of what it does. > > As long as your description of it only says how it is (ab)used in one > case, I will call it a hack, and a gross hack at that. > > > > > --- a/gcc/rtl.h > > > +++ b/gcc/rtl.h > > > @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { > > > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > > > > > /* 1 if RTX is a call_insn for a fake call. > > > - CALL_INSN use "used" flag to indicate it's a fake call. */ > > > + CALL_INSN use "used" flag to indicate it's a fake call. > > > + Used by the x86 vzeroupper instruction, > > > + in order to solve the problem of partial clobber registers, > > > + vzeroupper is defined as a call_insn with a special callee_abi, > > > + but it is not a real call and therefore has no function stack > > > + of its own. > > So because of this one thing (you need to insert partial clobbers) you > force all kinds of unrelated code to have changes, namely, code thatt > needs to do something with calls, but now you do not want to have that > doone on some calls because you promise that call will disappear > eventually, and it cannot cause any problems in the mean time? > > I am not convinced. This is not design, this is a terrible hack, this > is the opposite direction we should go in. Quote from https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570634.html > Also i grep CALL_P or CALL_INSN in GCC source codes, there are many > places which hold the assumption CALL_P/CALL_INSN is a real call. > Considering that vzeroupper is used a lot on the i386 backend, I'm a > bit worried that this implementation solution will be a bottomless > pit. Maybe, but I think the same is true for CLOBBER_HIGH. If we have a third alternative then we should consider it, but I think the call approach is still going to be less problematic then CLOBBER_HIGH. The main advantage of the call approach is that the CALL_P handling is (mostly) conservatively correct and performance problems are just a one-line change. The CLOBBER_HIGH approach instead requires changes to the way that passes track liveness information for non-call instructions (so is much more than a one-line change). Also, treating a CLOBBER_HIGH like a CLOBBER isn't conservatively correct, because other code might be relying on part of the register being preserved. > > > that doesn't set up a stack frame is fake as well? Maybe > > > > "CALL_INSN use "used" flag to indicate the instruction > > does not transfer control." > > > > thus that this call is not affecting regular control flow? (it might > > eventually still trap and thus cause non-call EH?) > > How it is used in shrink-wrap requires it to not have a stack frame (in > the compiler sense). > > > Not sure if "no function stack of its own" is a good constraint, > > vzeroupper does not perform any call or jump. > > Yeah. This stuff needs a rethink. > > What is wrong with just using an unspec and clobbers? > > > Segher
On Wed, Jul 7, 2021 at 4:15 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Jul 7, 2021 at 4:40 AM Hongtao Liu via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > On Tue, Jul 6, 2021 at 9:37 AM Hongtao Liu <crazylht@gmail.com> wrote: > > > > > > On Tue, Jul 6, 2021 at 7:31 AM Segher Boessenkool > > > <segher@kernel.crashing.org> wrote: > > > > > > > > Hi! > > > > > > > > I ran into this in shrink-wrap.c today. > > > > > > > > On Thu, Jun 03, 2021 at 02:54:07PM +0800, liuhongt via Gcc-patches wrote: > > > > > Use "used" flag for CALL_INSN to indicate it's a fake call. If it's a > > > > > fake call, it won't have its own function stack. > > > > > > > > Could you document somewhere what a "fake call" *is*? Including what > > > > that means to RTL, how this is expected to be used, etc.? In rtl.h is > > > fake call is used for TARGET_INSN_CALLEE_ABI, i'll add comments for > > > #define FAKE_CALL_P(RTX) in rtl.h > > > > > > Here's the patch I'm going to check in. > > > > Document FAKE_CALL_P in comments. > > > > gcc/ChangeLog: > > > > * rtl.h (FAKE_CALL_P): Add comments for FAKE_CALL_P. > > > > diff --git a/gcc/rtl.h b/gcc/rtl.h > > index 5ed0d6dd6fa..9afc60f08d8 100644 > > --- a/gcc/rtl.h > > +++ b/gcc/rtl.h > > @@ -840,7 +840,13 @@ struct GTY(()) rtvec_def { > > #define CALL_P(X) (GET_CODE (X) == CALL_INSN) > > > > /* 1 if RTX is a call_insn for a fake call. > > - CALL_INSN use "used" flag to indicate it's a fake call. */ > > + CALL_INSN use "used" flag to indicate it's a fake call. > > + Used by the x86 vzeroupper instruction, > > + in order to solve the problem of partial clobber registers, > > + vzeroupper is defined as a call_insn with a special callee_abi, > > + but it is not a real call and therefore has no function stack > > + of its own. > > I think that's a big vague - you could then say a sibling or tail call > to a function > that doesn't set up a stack frame is fake as well? Maybe > > "CALL_INSN use "used" flag to indicate the instruction > does not transfer control." > > thus that this call is not affecting regular control flow? (it might > eventually still trap and thus cause non-call EH?) > > Not sure if "no function stack of its own" is a good constraint, > vzeroupper does not perform any call or jump. > How about this? /* 1 if RTX is a call_insn for a fake call. - CALL_INSN use "used" flag to indicate it's a fake call. */ + CALL_INSN use "used" flag to indicate it's a fake call. + FIXME: it's only used by x86 vzeroupper to indicate it's + a normal instruction which doesn't have function stack + and doesn't impact control flow, and FAKE_CALL_P is not + handled thoroughly in the RTL, PR82735. */ > > + NB: FAKE_CALL_P is not handled thoroughly in the RTL. */ > > #define FAKE_CALL_P(RTX) \ > > (RTL_FLAG_CHECK1 ("FAKE_CALL_P", (RTX), CALL_INSN)->used) > > > > > > > > > > -- > > BR, > > Hongtao
On 7/7/2021 8:55 AM, Segher Boessenkool wrote: > On Mon, Jul 05, 2021 at 06:03:21PM -0600, Jeff Law wrote: >> It reminds me a bit of millicode calls on the PA or calls to special >> routines in libgcc. They're calls to functions, but those functions do >> not follow the standard ABI. > Something with CALL_INSN_FUNCTION_USAGE? And maybe some clobbers? I don't remember all the details on the PA side and the decision to express mul, div, mod as regular insns pre-dates my involvement in the PA port (hard to believe, but true). I'd hazard a guess the goal behind making them regular insns was to not inhibit leaf function detection, avoid caller-saves around the "calls" and such. CALL_INSN_FUNCTION_USAGE didn't exist until the mid 90s. I wouldn't be surprised if we could migrate the millicode calls to the CALL_INSN_FUNCTION_USAGE model. jeff
On Wed, Jul 07, 2021 at 11:23:48PM +0800, Hongtao Liu wrote: > On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: [ snip some old stuff ] > > Yeah. This stuff needs a rethink. > > > > What is wrong with just using an unspec and clobbers? > > > It's partial and **potential clobber**, All RTL "clobber" is always a potential clobber, it never guarantees the existing value does not survive. You can pass it through some unspec to make this more explicit. You will have to add some hook that CSE can use to figure out what bits are conserved by your target-specific construct, as you should have done in the first place. This will be much less work for you too, compared to actually checking if all existing GCC code needs too check "FAKE_CALL_P" or not (instead of just hoping it works now, as you do). Segher
On Wed, Jul 07, 2021 at 11:32:59PM +0800, Hongtao Liu wrote: > On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > So, a "FAKE_CALL" is very much a *real* call, on the RTL level, which is > > where we are here. But you want it to be treated differently because it > > will eventually be replaced by different insns. > It's CALL_INSN on the rtl level, but it's just a normal instruction > that it doesn't have a call stack, and it doesn't affect the control > flow There is no such thing as "call stack" (whatever that may mean) to do with the RTL "call" insn. How the return address is stored (if at all) is up to the target. Many do not store the return address on the stack (for example they have an RA or LR register for it). Those that do store it on a stack do not all change the stack pointer. In RTL, it *does* change the control flow. If you don't like that, don't use a "call" insn. You will have to update a *lot* more code than you did, otherwise. > > So because of this one thing (you need to insert partial clobbers) you > > force all kinds of unrelated code to have changes, namely, code thatt > > needs to do something with calls, but now you do not want to have that > > doone on some calls because you promise that call will disappear > > eventually, and it cannot cause any problems in the mean time? > > > > I am not convinced. This is not design, this is a terrible hack, this > > is the opposite direction we should go in. > > Quote from https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570634.html > > > Also i grep CALL_P or CALL_INSN in GCC source codes, there are many > > places which hold the assumption CALL_P/CALL_INSN is a real call. > > Considering that vzeroupper is used a lot on the i386 backend, I'm a > > bit worried that this implementation solution will be a bottomless > > pit. > > Maybe, but I think the same is true for CLOBBER_HIGH. If we have > a third alternative then we should consider it, but I think the > call approach is still going to be less problematic then CLOBBER_HIGH. > > The main advantage of the call approach is that the CALL_P handling > is (mostly) conservatively correct and performance problems are just > a one-line change. The CLOBBER_HIGH approach instead requires > changes to the way that passes track liveness information for > non-call instructions (so is much more than a one-line change). > Also, treating a CLOBBER_HIGH like a CLOBBER isn't conservatively > correct, because other code might be relying on part of the register > being preserved. And this isn't a one-line change either, and it is only partial already, and we don't know how deep the rabbit hole goes. Segher
On Thu, Jul 8, 2021 at 7:44 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Wed, Jul 07, 2021 at 11:23:48PM +0800, Hongtao Liu wrote: > > On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > [ snip some old stuff ] > > > > Yeah. This stuff needs a rethink. > > > > > > What is wrong with just using an unspec and clobbers? > > > > > It's partial and **potential clobber**, > > All RTL "clobber" is always a potential clobber, it never guarantees the > existing value does not survive. You can pass it through some unspec to > make this more explicit. You will have to add some hook that CSE can TARGET_INSN_CALLEE_ABI is the hook designed for this.
On Thu, Jul 8, 2021 at 7:56 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Wed, Jul 07, 2021 at 11:32:59PM +0800, Hongtao Liu wrote: > > On Wed, Jul 7, 2021 at 10:54 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > So, a "FAKE_CALL" is very much a *real* call, on the RTL level, which is > > > where we are here. But you want it to be treated differently because it > > > will eventually be replaced by different insns. > > It's CALL_INSN on the rtl level, but it's just a normal instruction > > that it doesn't have a call stack, and it doesn't affect the control > > flow > > There is no such thing as "call stack" (whatever that may mean) to do > with the RTL "call" insn. How the return address is stored (if at all) > is up to the target. Many do not store the return address on the stack > (for example they have an RA or LR register for it). Those that do > store it on a stack do not all change the stack pointer. > > In RTL, it *does* change the control flow. If you don't like that, > don't use a "call" insn. You will have to update a *lot* more code > than you did, otherwise. > > > > So because of this one thing (you need to insert partial clobbers) you > > > force all kinds of unrelated code to have changes, namely, code thatt > > > needs to do something with calls, but now you do not want to have that > > > doone on some calls because you promise that call will disappear > > > eventually, and it cannot cause any problems in the mean time? > > > > > > I am not convinced. This is not design, this is a terrible hack, this > > > is the opposite direction we should go in. > > > > Quote from https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570634.html > > > > > Also i grep CALL_P or CALL_INSN in GCC source codes, there are many > > > places which hold the assumption CALL_P/CALL_INSN is a real call. > > > Considering that vzeroupper is used a lot on the i386 backend, I'm a > > > bit worried that this implementation solution will be a bottomless > > > pit. > > > > Maybe, but I think the same is true for CLOBBER_HIGH. If we have > > a third alternative then we should consider it, but I think the > > call approach is still going to be less problematic then CLOBBER_HIGH. > > > > The main advantage of the call approach is that the CALL_P handling > > is (mostly) conservatively correct and performance problems are just > > a one-line change. The CLOBBER_HIGH approach instead requires > > changes to the way that passes track liveness information for > > non-call instructions (so is much more than a one-line change). > > Also, treating a CLOBBER_HIGH like a CLOBBER isn't conservatively > > correct, because other code might be relying on part of the register > > being preserved. > > And this isn't a one-line change either, and it is only partial already, > and we don't know how deep the rabbit hole goes. maybe, and if there's existed infrastructure to solve vzeroupper issue, i'm ok to change my patch. > > > Segher
diff --git a/gcc/df-scan.c b/gcc/df-scan.c index 6691c3e8357..1268536b3f0 100644 --- a/gcc/df-scan.c +++ b/gcc/df-scan.c @@ -3090,7 +3090,8 @@ df_get_call_refs (class df_collection_rec *collection_rec, for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) { - if (i == STACK_POINTER_REGNUM) + if (i == STACK_POINTER_REGNUM + && !FAKE_CALL_P (insn_info->insn)) /* The stack ptr is used (honorarily) by a CALL insn. */ df_ref_record (DF_REF_BASE, collection_rec, regno_reg_rtx[i], NULL, bb, insn_info, DF_REF_REG_USE, diff --git a/gcc/final.c b/gcc/final.c index e0a70fcd830..817f7722cb2 100644 --- a/gcc/final.c +++ b/gcc/final.c @@ -4109,7 +4109,8 @@ leaf_function_p (void) for (insn = get_insns (); insn; insn = NEXT_INSN (insn)) { if (CALL_P (insn) - && ! SIBLING_CALL_P (insn)) + && ! SIBLING_CALL_P (insn) + && ! FAKE_CALL_P (insn)) return 0; if (NONJUMP_INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE diff --git a/gcc/reg-stack.c b/gcc/reg-stack.c index 25210f0c17f..1d9ea035cf4 100644 --- a/gcc/reg-stack.c +++ b/gcc/reg-stack.c @@ -174,6 +174,7 @@ #include "reload.h" #include "tree-pass.h" #include "rtl-iter.h" +#include "function-abi.h" #ifdef STACK_REGS @@ -2368,6 +2369,18 @@ subst_asm_stack_regs (rtx_insn *insn, stack_ptr regstack) } } } + +/* Return true if a function call is allowed to alter some or all bits + of any stack reg. */ +static bool +callee_clobbers_any_stack_reg (const function_abi & callee_abi) +{ + for (unsigned regno = FIRST_STACK_REG; regno <= LAST_STACK_REG; regno++) + if (callee_abi.clobbers_at_least_part_of_reg_p (regno)) + return true; + return false; +} + /* Substitute stack hard reg numbers for stack virtual registers in INSN. Non-stack register numbers are not changed. REGSTACK is the @@ -2382,7 +2395,10 @@ subst_stack_regs (rtx_insn *insn, stack_ptr regstack) bool control_flow_insn_deleted = false; int i; - if (CALL_P (insn)) + /* If the target of the call doesn't clobber any stack registers, + Don't clear the arguments. */ + if (CALL_P (insn) + && callee_clobbers_any_stack_reg (insn_callee_abi (insn))) { int top = regstack->top; diff --git a/gcc/rtl.c b/gcc/rtl.c index b0ba1ff684c..aaee882f5ca 100644 --- a/gcc/rtl.c +++ b/gcc/rtl.c @@ -395,8 +395,10 @@ shallow_copy_rtx (const_rtx orig MEM_STAT_DECL) case SCRATCH: break; default: - /* For all other RTXes clear the used flag on the copy. */ - RTX_FLAG (copy, used) = 0; + /* For all other RTXes clear the used flag on the copy. + CALL_INSN use "used" flag to indicate it's a fake call. */ + if (!INSN_P (orig)) + RTX_FLAG (copy, used) = 0; break; } return copy; diff --git a/gcc/rtl.h b/gcc/rtl.h index 35178b5bfac..5ed0d6dd6fa 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -839,6 +839,11 @@ struct GTY(()) rtvec_def { /* Predicate yielding nonzero iff X is a call insn. */ #define CALL_P(X) (GET_CODE (X) == CALL_INSN) +/* 1 if RTX is a call_insn for a fake call. + CALL_INSN use "used" flag to indicate it's a fake call. */ +#define FAKE_CALL_P(RTX) \ + (RTL_FLAG_CHECK1 ("FAKE_CALL_P", (RTX), CALL_INSN)->used) + /* Predicate yielding nonzero iff X is an insn that cannot jump. */ #define NONJUMP_INSN_P(X) (GET_CODE (X) == INSN) diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c index ba7b5cd56fd..5e60f34f749 100644 --- a/gcc/shrink-wrap.c +++ b/gcc/shrink-wrap.c @@ -57,7 +57,7 @@ requires_stack_frame_p (rtx_insn *insn, HARD_REG_SET prologue_used, HARD_REG_SET hardregs; unsigned regno; - if (CALL_P (insn)) + if (CALL_P (insn) && !FAKE_CALL_P (insn)) return !SIBLING_CALL_P (insn); /* We need a frame to get the unique CFA expected by the unwinder. */