Re: [Tinycc-devel] [PATCH] deprecating VT_REF

[Michael Matz]

Hi,

On Sat, 21 Feb 2015, Edmund Grimley Evans wrote:

> Michael Matz <address@hidden>:
>
> > I don't like this.  tccgen.c should become _less_ dependend on the
> > TARGET defines, not more.  Hence either VT_REF has a purpose and it
> > might make sense to use it in more backends, or it hasn't and should
> > be removed also from x86_64.
>
> I agree, but inserting those #ifdefs is merely making the
> architecture-specificity explicit, thereby perhaps encouraging someone
> to do something about it. Without the #ifdefs, but with VT_REF only
> used by the x86_64 back end, you have hidden target-specificity, which
> is more dangerous.

I disagree about dangerous; it's only a problem for those targets, and 
presumably for them the generic handling of VT_REF is correct.  If you 
mean dangerous as in "others might be tempted to use it in their target, 
even though the generic handling isn't correct for them", then, well, life 
is tough; it's not enough reason IMHO to uglify common code.  Neither is 
trying to make others interested in removing the thing.

But apart from that philosophical argument, I claim that the concept of 
VT_REF is not target specific at all.  It does have things in common with 
VT_LLOCAL, but is not _quite_ the same currently, as we now found out in 
the other mails.  This not-quite-equality might indeed be bugs in how 
VT_LLOCAL is handled, e.g. VT_LLOCAL might be understood as an 
optimization to VT_REF (even though the latter was introduced later), the 
optimization being that lvalueness/referenceness can be changed by a 
simple bitflip, instead of generating code.

So, yes, ideally VT_LLOCAL and VT_REF should be merged.  Though the 
question still is into which one.  VT_LLOCAL is tricky, as the docu said 
:)  And it currently contains bugs, when used for some things.

_If_ the issues with VT_LLOCAL can be fixed, then I'd be in favor of 
removing VT_REF.  After such fixing goes in.

> > AFAIU VT_REF is added when an argument is passed in a caller
> > allocated buffer whose address itself is passed in a register (i.e.
> > by-reference-passing).
>
> Do you mean on the stack?

Usually, but not necessarily.  For a function like
  int f (int a; struct large b) { ... }
there are two ways to get to 'b':
1) it's saved on the stack by caller, the address will be right below
   the slot for 'a', or the first slot, if 'a' gets a register.  So
   addressof(b) will be something like sp+offset.
2) it's stored in some buffer (not necessarily stack) and the address of
   it is explicitely passed in the argument that would take the place of
   'b' were it of pointer type.  Now addressof(b) is not necessarily some
   offset from sp, but rather must be loaded (either from a register,
   or the stack slot that addressof value got)

Yes, VT_LLOCAL can nearly express the concept 2.  VT_REF as well (just 
that it works right now).

> On arm64 the caller sometimes allocates a buffer then passes a pointer
> to that buffer on the stack, and I'm handling that with VT_LLOCAL,

Okay, that's the same as win64 then, which means you could have also used 
VT_REF.  (I'm not saying you should have, just stating the fact.)

> which is already widely used. (I checked that a lot of VT_LLOCALs are
> generated when you run the test suite on i386, for example.)

Yes, VT_LLOCAL is used often.  But it's used for only one thing currently 
(i.e. the number of concepts expressed by it is not very large): when an 
lvalue contained in a register is spilled, it becomes a stack slot with 
VT_LLOCAL.  That makes sense, because "lvalue in a register" means the 
register contains the address of that lvalue (and then so does the stack 
slot).  But for instance it's not totally clear what VT_LLOCAL means on 
something that never was a register (e.g. because it is not of right 
type).  Also, is it ever okay to have VT_LLOCAL without VT_LVAL?  (There 
should be nothing wrong about this, but does the code always handle this?)

I think perhaps it's time to try to figure out what the three flags 
(VT_LLOCAL, VT_LVAL and VT_LOCAL) really mean, which combinations make 
sense (under which circumstances) and which operations do what to the 
flags.  If we had clear rules we probably can make VT_LLOCAL do exactly 
what's necessary in generic code, to make it safely and generally usable.
Right now the whole VT_LLOCAL handling seems quite ad-hoc (and that 
results exactly in the omissions you saw with the '.' operator).

> I'd definitely prefer it if someone could remove VT_REF altogether. 
> Alternatively, someone could patch the documentation to explain exactly 
> what VT_REF means, why it's needed, and how it differs from VT_LLOCAL. 
> If neither of those patches is forthcoming I'd prefer something like my 
> patch to be committed to just leaving it as it is.

Well, we're still discussing, so not all is lost yet :)  I'd actually hope 
to have such description for VT_LLOCAL in the end, and remove VT_REF after 
the current threads are finished.


Ciao,
Michael.