516855 - nanojit: Templatize lir->insLoad somehow so that the size of the field being read is determined implicitly

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The current idiom of:

  struct Object
  {
    void* data;
  };

  lir->insLoad(LIR_ldp, objIns, ins->insImm(offsetof(Object, data)));

is fragile and breaks if you forget to use LIR_ldp, change the type of the data member, etc.  It should be possible somehow to templatize this so that the size of the field being loaded determines the instruction used.  My template- and pointer-to-member-fu is not quite up to the task of sketching out an API that might possibly compile or nearly-compile, but I'm sure it's possible.

This was apparently a pervasive problem during porting of TraceMonkey to 64-bit; it seems like it would be a great idea to entirely replace the troublesome API with a safe one.

https://developer.mozilla.org/En/SpiderMonkey/Internals/64-bit_Compatibility#Loading_and_Storing_Native_Integers

Comment 1

[Nicholas Nethercote [inactive]]

insStorei() already has this behaviour -- you don't have to specify which store opcode you want.  So it shouldn't be hard to do something similar for loads.   It doesn't involve anything fancy, it just looks at the operand's opcode in LirBufWriter::insStorei(). 

Having said that, the current type-sniffing is a bit hacky and I have plans for something more principled for bug 520714.

Also, we'll need to keep an explicit-opcode version around for the CSEable loads (ldc, ldqc, ldcb, etc).

Comment 2

[Jeff Walden [:Waldo]]

Fancy could get you faster compile-time dispatch.  But if we can get safety without it, that's enough for the problems raised here.

Why is an explicit-opcode version needed?  Why wouldn't an alternative "insConstLoadi" address the distinction?

Comment 3

[Nicholas Nethercote [inactive]]

Hmm, I'm mistaken.  This is quite different, and harder, than the insStorei() case where the type can be gleaned easily from the LIns*.  Sorry for the noise.

Comment 4

[Luke Wagner [:luke]]

Attachment: prototype

Cool idea Waldo!  I attached an example of how this might be done as well as some uses.

There is one weakness I can see, though: there is no simple way to distinguish a fixed-size integral type from a polymorphic one.  E.g., let's say you start with:

    lir->insLoad(LIR_ldp, v_ins, offsetof(JSString, mLength))

if you write:

    INS_INFER_MEMBER_LOAD(v_ins, JSString, mLength)

then the type deduction will see that JSString::mLength is, on a 32-bit system, an unsigned int and choose the LIR_ld opcode or, on a 64-bit system, see an unsigned long long and insert a LIR_ldq.  Both are wrong, since we want a LIR_ldp.  This also arises with JSObject::classword.

There's also the issue of how to pick LIR_ldp vs LIR_ldcp...

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)

> if you write:
> 
>     INS_INFER_MEMBER_LOAD(v_ins, JSString, mLength)
> 
> then the type deduction will see that JSString::mLength is, on a 32-bit system,
> an unsigned int and choose the LIR_ld opcode or, on a 64-bit system, see an
> unsigned long long and insert a LIR_ldq.  Both are wrong, since we want a
> LIR_ldp.  This also arises with JSObject::classword.

Why is that wrong?  ldp is just a synonym for ld on 32-bit systems and ldq on 64-bit systems.

> There's also the issue of how to pick LIR_ldp vs LIR_ldcp...

I figure the ldcp-type cases would still be done by explicitly providing the opcode.  The inferral would only be for the vanilla loads: ld, ldq, ldf (once it's added).

Comment 6

[Edwin Smith]

at least in principle its possible to have INS_INFER_MEMBER_LOAD_CSE(...) with the range of CSE load ops.  const could be a clue to drive the template inference, but many of our struct member loads in Tamarin are cse-able even though the field are not declared const.

Comment 7

[Luke Wagner [:luke]]

(In reply to comment #5)
> Why is that wrong?  ldp is just a synonym for ld on 32-bit systems and ldq on
> 64-bit systems.

I had assumed (perhaps just superstitiously) that there was some amount of runtime type checking that would complain.  E.g., on a 32-bit build, is it fine to pass a LIR_ld to a LIR_piadd?  If so, awesome!

Comment 8

[Jeff Walden [:Waldo]]

The prototype looks nice, but I think I'm missing why macros are necessary.  This (with no macros) is what I had in mind:

     1	#include <stdio.h>
     2	#include <stdint.h>
     3	
     4	template<class C, typename T>
     5	void
     6	Load(C* val, T C::* member)
     7	{
     8	  C* c = static_cast<C*>(0);
     9	  printf("sizeof(val->*member): %d\n",
    10	         sizeof(val->*member));
    11	  printf("offsetof(C, C::member): %d\n",
    12	         uintptr_t(&(c->*member)));
    13	}
    14	
    15	struct A { int i; char c; };
    16	struct B { double d; };
    17	
    18	int main()
    19	{
    20	  A a;
    21	  B b;
    22	
    23	  Load(&a, &A::c);
    24	  Load(&b, &B::d);
    25	}
sizeof(val->*member): 1
offsetof(C, C::member): 4
sizeof(val->*member): 8
offsetof(C, C::member): 0

(I'm aware it relies on undefined behavior at at least one point, but I'm willing to hazard that as normal compilers will behave in the expected way.)

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
>
> I had assumed (perhaps just superstitiously) that there was some amount of
> runtime type checking that would complain.  E.g., on a 32-bit build, is it fine
> to pass a LIR_ld to a LIR_piadd?

Yes.  All the 'p'-sized opcodes are just synonyms.  See the definition of enum LOpcode in nanojit/Native.h.

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #8)
> The prototype looks nice, but I think I'm missing why macros are necessary. 
> ...
> (I'm aware it relies on undefined behavior at at least one point...

I think you have demonstrated aptly why (I thought) macros were necessary ;)

> but I'm willing to hazard that as normal compilers will behave in the expected way.)

I don't have real-world cross-compiler experience with this trick, which is why I avoided it, but maybe you are right and this is one of these "thou shalt not break".

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.