Re: [PATCH 1/2] CALL_INSN may not be a real function call.

[Segher Boessenkool <segher@kernel.crashing.org>]

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