Last Comment Bug 685838 - IonMonkey: Implement JSOP_NEWARRAY
: IonMonkey: Implement JSOP_NEWARRAY
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on: 680315 688174
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-09-09 05:38 PDT by Nicolas B. Pierron [:nbp]
Modified: 2011-12-06 15:11 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[0001] Add LCallInstructionHelper to inform register allocators about calls. (6.30 KB, patch)
2011-09-29 05:18 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
[0002] Handle LNewArray bytecode instruction. (14.99 KB, patch)
2011-09-29 05:19 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
[0002] Handle LNewArray bytecode instruction. (14.90 KB, patch)
2011-10-03 10:54 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Handle LNewArray bytecode instruction. (28.79 KB, patch)
2011-11-11 17:27 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Handle LNewArray bytecode instruction. (36.95 KB, patch)
2011-11-15 19:36 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Handle LNewArray bytecode instruction. (41.66 KB, patch)
2011-12-01 11:34 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Handle LNewArray bytecode instruction. (46.74 KB, patch)
2011-12-02 11:27 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Handle LNewArray bytecode instruction. (52.99 KB, patch)
2011-12-03 15:23 PST, Nicolas B. Pierron [:nbp]
sstangl: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-09-09 05:38:54 PDT
Implement the opcode JSOP_NEwARRAY in ionmonkey.
Comment 1 Nicolas B. Pierron [:nbp] 2011-09-29 05:18:59 PDT
Created attachment 563373 [details] [diff] [review]
[0001] Add LCallInstructionHelper to inform register allocators about calls.
Comment 2 Nicolas B. Pierron [:nbp] 2011-09-29 05:19:55 PDT
Created attachment 563376 [details] [diff] [review]
[0002] Handle LNewArray bytecode instruction.
Comment 3 Nicolas B. Pierron [:nbp] 2011-10-03 10:54:13 PDT
Created attachment 564257 [details] [diff] [review]
[0002] Handle LNewArray bytecode instruction.

Add missing include to avoid warning with inline functions.
Comment 4 David Anderson [:dvander] 2011-10-05 14:24:13 PDT
Comment on attachment 563373 [details] [diff] [review]
[0001] Add LCallInstructionHelper to inform register allocators about calls.

Review of attachment 563373 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonLIR.h
@@ +592,5 @@
> +    virtual bool makeCall() const {
> +        return false;
> +    };
> +    virtual uint32 grpDefs() const {
> +        return 0;

Some naming nits:
  s/makeCall/isCall/
  s/grpDefs/gprDefs/

Bonus points if these return TypedRegister sets instead (maybe even just one RegisterSet for both kinds).

@@ +752,5 @@
> +
> +
> +template <uint32 GprMask, uint32 FpuMask, size_t Operands, size_t Temps>
> +class LCallInstructionHelper : public LInstructionHelper<
> +    BIT_COUNT(GprMask) + BIT_COUNT(FpuMask), Operands, Temps>

One thing to consider is that we only have two kinds of calls - and we want to move to just one very soon (ion->ion calls *should* use the native ABI, so then we would only have one call type).

It might be simpler to just expose two virtual functions: isCall() and isExternalCall(), and have the register allocator deal with it, since this abstraction will see pretty limited use.
Comment 5 David Anderson [:dvander] 2011-10-05 15:19:56 PDT
Comment on attachment 563376 [details] [diff] [review]
[0002] Handle LNewArray bytecode instruction.

Review of attachment 563376 [details] [diff] [review]:
-----------------------------------------------------------------

One thing I realized reading this patch is that, I think the definitions of a call instruction are not necessarily the same as its callVM() return set. For example, NewArray wants to return just an object pointer, but the callVM returns a Value.

::: js/src/ion/IonBuilder.cpp
@@ +1753,5 @@
> +IonBuilder::jsop_newarray(uint32 size)
> +{
> +    MNewArray *ins = new MNewArray(size, &stubs::NewInitArray_vm);
> +    if (!ins)
> +        return false;

No need to NULL check here, most MInstructions have infallible new.

@@ +1758,5 @@
> +
> +    current->add(ins);
> +
> +    // Type inference of array initialization.
> +    // ins->infer(oracle->binaryOp(script, pc));

You can get rid of this - we'll need to do *something* here for the types::TypeSet, but we don't have to worry about that yet.

::: js/src/ion/IonStubCalls.cpp
@@ +59,5 @@
> +    if (!obj)
> +        return MagicValue(JS_GENERIC_ERROR);
> +
> +    return ObjectValue(*obj);
> +}

It's exciting to finally have JIT C++ calls that don't obey some extremely weird convention. But we've already run into the problem that has plagued JM: stub calls often don't have parity with the interpreter (here, we need to get a TypeObject), and in general as the interpreter evolves on m-c the ionmonkey branch will get left behind.

> // :TODO: (Bug ???) Merge this file with jsinterp.cpp and

After talking with Luke, this is definitely the right idea. Let's just do it now :) and in fact, let's do it on m-c! In jsinterp.h/cpp we could have:

JSObject *NewInitArray(JSContext *cx, uint32 count, types::TypeSet *types);

In the interpreter, JSOP_NEWARRAY would become:
    TypeObject *type = TypeScript::InitObject(cx, script, regs.pc, JSProto_Array);
    if (!type)
        ...
    JSObject *obj = NewInitArray(cx, GET_UINT24(regs.pc), types);
    if (!obj...

Similarly, methodjit/Stubcalls.cpp stubs::NewInitArray would be simplified. Now, the Ion version can just call into jsinterp -- we can compute the TypeObject during MIR building, attaching to the MNewArray. Then this pointer can be hardcoded in the code generator:
   masm.push(lir->mir()->count());
   masm.push(lir->mir()->typeObject());

If we land this minor refactoring on mozilla-central, then we may never need IonStubCalls.cpp and we can put off parity problems.

::: js/src/ion/LIR-Common.h
@@ +169,5 @@
> +    Registers::JSCCallMask, FloatRegisters::JSCCallMask, 0, 0>
> +{
> +    // Number of space to allocate for the array.
> +    uint32 count_;
> +    const VMFunction *fun_;

Do you need to store a VMFunction here? In theory it should be reachable inside the code generator.

::: js/src/ion/MIR.h
@@ +809,5 @@
> +
> +    MNewArray(uint32 count, const VMFunction *fun)
> +        : count_(count), fun_(fun)
> +    {
> +        setResultType(MIRType_Value);

NewArray should return an Object. A Value technically works here, but it'll get passed around as a wider type than needed, and may be unnecessarily unboxed with a type guard.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +689,5 @@
> +CodeGeneratorX86Shared::visitNewArray(LNewArray *builder)
> +{
> +    masm.push(Imm32(builder->count()));
> +    if (!callVM(builder->getFunction(), builder->snapshot()))
> +        return false;

A few ways of approaching the unbox here:
 (1) Explicitly insert an MUnbox into the MIR. That seems kind of sad.
 (2) Explicitly insert an unbox after callVM.
 (3) Add a JSValueType return type to callVM, have callVM do an optional unbox.
 (4) Add a new convention to callVM that lets you return pointer-or-NULL and
     considers NULL to be an error. Then we could support both:

Value v = call();
if (v.isError()) ...

JSObject *obj = call();
if (!obj) ...
Comment 6 Nicolas B. Pierron [:nbp] 2011-10-06 04:02:18 PDT
(In reply to David Anderson [:dvander] from comment #4)
> Comment on attachment 563373 [details] [diff] [review] [diff] [details] [review]
> [0001] Add LCallInstructionHelper to inform register allocators about calls.
> 
> Review of attachment 563373 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> @@ +752,5 @@
> > +
> > +
> > +template <uint32 GprMask, uint32 FpuMask, size_t Operands, size_t Temps>
> > +class LCallInstructionHelper : public LInstructionHelper<
> > +    BIT_COUNT(GprMask) + BIT_COUNT(FpuMask), Operands, Temps>
> 
> One thing to consider is that we only have two kinds of calls - and we want
> to move to just one very soon (ion->ion calls *should* use the native ABI,
> so then we would only have one call type).
> 
> It might be simpler to just expose two virtual functions: isCall() and
> isExternalCall(), and have the register allocator deal with it, since this
> abstraction will see pretty limited use.

As you mention in the previous review, we might want to deal with different kind of call convention, when one return a Value on 2 registers or an object in one register.  The LCallInstructionHelper handle such case by using the bit mask to determine the number of definitions of the LInstructionHelper.  Indeed the number of variation of Mask would be small.

I still prefer to keep the register mask/set close to the LIR and avoid moving them inside the register allocators.  This would make the register allocators easier to maintain.
Comment 7 Nicolas B. Pierron [:nbp] 2011-11-11 17:27:10 PST
Created attachment 573971 [details] [diff] [review]
Handle LNewArray bytecode instruction.

rebased, use interpreter functions.
Comment 8 David Anderson [:dvander] 2011-11-11 22:46:56 PST
Comment on attachment 573971 [details] [diff] [review]
Handle LNewArray bytecode instruction.

Review of attachment 573971 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good, but deferring r? from two things below: (1) TypeOracle abstractions, and (2) I would like to simplify the way callable LIR is declared. It might help to not worry about abstracting it too much yet. NewArray doesn't have many requirements, so we can grow the functionality incrementally.

::: js/src/ion/IonBuilder.cpp
@@ +1846,5 @@
> +IonBuilder::jsop_newarray(uint32 size)
> +{
> +    using namespace types;
> +
> +    TypeObject *type = TypeScript::InitObject(cx, script, pc, JSProto_Array);

This should be abstracted through the TypeOracle. I'm not sure what the best API is there - might want to confer with jandem or bhackett. Ideally IM and TypeInference should be as separated as possible.

It can be really cheesy, even:

...
    TypeObject *initializationObject(JSScript *script, jsbytecode *pc, JSWhatever protoKey);

::: js/src/ion/IonLIR.h
@@ +602,5 @@
> +    virtual const RegisterSet& spillRegs() const {
> +        JS_NOT_REACHED("Should never be called when isCall return false.");
> +        static const RegisterSet s;
> +        return s;
> +    }

spillRegs should always be "all registers", so this is unnecessary.

@@ +808,5 @@
> +template <enum VMFunction::ReturnType DefType, size_t Operands, size_t Temps>
> +class LVMCallInstructionHelper : public LCallInstructionHelper<
> +    VMDefMask<DefType>::GprMask, VMDefMask<DefType>::FpuMask, Operands, Temps>
> +{
> +};

This is a neat trick, but it strikes me as something that could be made very simple by sniffing it out in the register allocators. We're always returning either one word (in eax or rax) or two (in ecx+edx or rcx), so the register allocator just needs to look at the number of defs.

If it helps, we should change the Value return types to eax+edx and rax. Then the register allocator changes become 3-4 lines of code.

::: js/src/ion/LinearScan.cpp
@@ +481,5 @@
>          // point of definition, if found.
>          for (LInstructionReverseIterator ins = block->rbegin(); ins != block->rend(); ins++) {
>              // Calls may clobber registers, so force a spill and reload around the callsite.
> +            if (ins->isCall())
> +                for (AnyRegisterIterator iter(ins->spillRegs()); iter.more(); iter++)

Nit: Nice simplification, but the |if| should be braced because its body is >1 lines.

::: js/src/ion/Lowering.cpp
@@ +93,5 @@
> +    LNewArray *lir = new LNewArray(mir);
> +
> +    if (!defineVMReturn(lir, mir))
> +        return false;
> +    if (!assignSnapshot(lir))

With your Safepoints patch, this should become "assignSafepoint", right?
Comment 9 Nicolas B. Pierron [:nbp] 2011-11-14 15:28:48 PST
(In reply to David Anderson [:dvander] from comment #8)
> ::: js/src/ion/IonLIR.h
> @@ +602,5 @@
> > +    virtual const RegisterSet& spillRegs() const {
> > +        JS_NOT_REACHED("Should never be called when isCall return false.");
> > +        static const RegisterSet s;
> > +        return s;
> > +    }
> 
> spillRegs should always be "all registers", so this is unnecessary.

spillRegs has no reasons to be called at this stage, so having the NOT_REACHED assertion seems safer for now.

> @@ +808,5 @@
> > +template <enum VMFunction::ReturnType DefType, size_t Operands, size_t Temps>
> > +class LVMCallInstructionHelper : public LCallInstructionHelper<
> > +    VMDefMask<DefType>::GprMask, VMDefMask<DefType>::FpuMask, Operands, Temps>
> > +{
> > +};
> 
> This is a neat trick, but it strikes me as something that could be made very
> simple by sniffing it out in the register allocators. We're always returning
> either one word (in eax or rax) or two (in ecx+edx or rcx), so the register
> allocator just needs to look at the number of defs.

The current implementation is not complete, because one may call a VM function and have additional definitions.

Inferring the mask from the number of definition is mostly wrong and duplicate the sources of information and potentially the number of clash caused by inconsistencies between expected registers of the VMFunction wrapper and mask return by spillRegs.

> ::: js/src/ion/Lowering.cpp
> @@ +93,5 @@
> > +    LNewArray *lir = new LNewArray(mir);
> > +
> > +    if (!defineVMReturn(lir, mir))
> > +        return false;
> > +    if (!assignSnapshot(lir))
> 
> With your Safepoints patch, this should become "assignSafepoint", right?

It should if this patch is rebased on top of the safepoint patch.
Comment 10 Nicolas B. Pierron [:nbp] 2011-11-15 19:36:28 PST
Created attachment 574779 [details] [diff] [review]
Handle LNewArray bytecode instruction.

- Rebase on latest modifications.
- Reduce noice caused by similar register masks.
- Augment VMFunction structure to have clear meaning between the properties of the C function and the properties of the wrapper.  Update the trampoline code.
- Fix x86 Trampoline error (cx == ionJSContext not ionTop)
- Update call convention in callVM -- the Z flag is no longer part of the call convention because the code use an exception mechanism now.
Comment 11 David Anderson [:dvander] 2011-11-16 00:15:54 PST
> Inferring the mask from the number of definition is mostly wrong and
> duplicate the sources of information and potentially the number of clash
> caused by inconsistencies between expected registers of the VMFunction
> wrapper and mask return by spillRegs.

Why is it wrong? And if it's wrong, is there something else we can do? Basically, I don't understand why spillRegs is necessary. There is a lot of complexity here and it seems like it could be much simpler. There are only two return types (word, or value) - maybe "double" in the future - this can all be inferred from the LIR and the registers involved in all are fixed. This inference could even be shared in LInstruction, and could be inside spillRegs, so it wouldn't have to be duplicated across RAs.
Comment 12 Nicolas B. Pierron [:nbp] 2011-11-16 10:03:34 PST
(In reply to David Anderson [:dvander] (offline until nov 21) from comment #11)
> > Inferring the mask from the number of definition is mostly wrong and
> > duplicate the sources of information and potentially the number of clash
> > caused by inconsistencies between expected registers of the VMFunction
> > wrapper and mask return by spillRegs.
> 
> Why is it wrong? And if it's wrong, is there something else we can do?

It's wrong because you have 2 sources of informations, one is the VMFunction/Mask of definined registers and the other is the number of definitions.  In the current implementation you have only one source of information which is either the VMFunction or the Mask of defined registers.

So I prefer to avoid future unconsistencies, such as the JSCCallMask (which changed 4 times last week).

One example, is that I may want MNewArray to have an additional definition.  This additional definition is to transmit the address of the "slots" field of the returned JSObject pointer.  The advantage is that following JSOP_INITELEM won't have to fetch this slot at each assignment of the table.

So inferring the return value based on the number of definition *is* a bad idea, because this prevent us to do such kind of optimization.

Of course many other optimizations are possible for the initialization of multiple fields of arrays.

> Basically, I don't understand why spillRegs is necessary. There is a lot of
> complexity here and it seems like it could be much simpler. There are only
> two return types (word, or value) - maybe "double" in the future - this can
> all be inferred from the LIR and the registers involved in all are fixed.

There is one thing I don't understand with this spillRegs, why do we care to create the mask of allocatable register private from the definition mask ?  Couldn't we spill all allocatable registers because this is what would happened at the end ?

> This inference could even be shared in LInstruction, and could be inside
> spillRegs, so it wouldn't have to be duplicated across RAs.

I don't get it ?  This is already the case.
Comment 13 Nicolas B. Pierron [:nbp] 2011-11-21 19:36:58 PST
NewArray patch is used as a base for a test case running both safepoint and newarray.

https://tbpl.mozilla.org/?tree=Try&rev=be221e2506c7
Comment 14 Nicolas B. Pierron [:nbp] 2011-12-01 11:34:00 PST
Created attachment 578341 [details] [diff] [review]
Handle LNewArray bytecode instruction.

Modification since last patch:
- Handle VMFunction::ReturnNothing in defineVMReturn.
- Move the VMFunction definition to the CodeGenerator cpp file. (close to the stack manipulation)
- Spill all allocatable registers for calls. (sad)
- Replace computation of the number of Def based on the mask by an assertion in defineVMReturn.
- Remove template mechanism to determine spilled registers based on the Return type of the wrapper.
- Use HeapPtr for TypeObject (in MNewArray).
- Do not copy MIR content into the LIR.
- Only use volatile registers inside C wrappers.
Comment 15 Sean Stangl [:sstangl] 2011-12-01 16:15:03 PST
Comment on attachment 578341 [details] [diff] [review]
Handle LNewArray bytecode instruction.

Review of attachment 578341 [details] [diff] [review]:
-----------------------------------------------------------------

Rushed comments at the end, since I was informed that this patch is now obsolete.

::: js/src/ion/GreedyAllocator.cpp
@@ +590,2 @@
>  {
> +    for (AnyRegisterIterator iter; iter.more(); iter++) {

This will evict more registers than necessary for JS calls. If an instruction isCall(), it should remember the appropriate register mask to use. This is a performance degradation for no benefit.

::: js/src/ion/IonBuilder.cpp
@@ +2253,5 @@
> +    using namespace types;
> +
> +    // Do not use the oracle here because this type object will stay the same for
> +    // each (script, pc, kind = Array).
> +    TypeObject *type = TypeScript::InitObject(cx, script, pc, JSProto_Array);

I don't understand the comment. Why would using an oracle be *wrong*? How does this behave without -n?

::: js/src/ion/IonLIR.h
@@ +783,5 @@
> +template <enum VMFunction::ReturnType DefType, size_t Defs, size_t Operands, size_t Temps>
> +class LVMCallInstructionHelper : public LCallInstructionHelper<Defs, Operands, Temps>
> +{
> +  public:
> +    LVMCallInstructionHelper(const VMFunction& f) {

nit: House style seems to be "const VMFunction &f".

::: js/src/ion/IonRegisters.h
@@ +213,5 @@
>      }
>      static inline TypedRegisterSet Not(const TypedRegisterSet &in) {
>          return TypedRegisterSet(~in.bits_ & T::Codes::AllocatableMask);
>      }
> +    static inline TypedRegisterSet VolatileNot(const TypedRegisterSet &in) {

This is a bit fishy. Maybe the mask to use should be part of the template?

@@ +394,5 @@
>        : geniter_(genset), floatiter_(floatset)
>      { }
> +    AnyRegisterIterator(const RegisterSet &set)
> +      : geniter_(set.gpr_), floatiter_(set.fpu_)
> +    { }

This is nice.

::: js/src/ion/LIR-Common.h
@@ +183,5 @@
>      }
>  };
>  
> +typedef LVMCallInstructionHelper<VMFunction::ReturnPointer, 1, 0, 0> LNewArrayBase;
> +class LNewArray : public LNewArrayBase

Can we remove the typedef and have the ":" on a newline with a 2-space indentation, if it's breaking the 90-char limit?

@@ +188,5 @@
> +{
> +  public:
> +    LIR_HEADER(NewArray);
> +
> +    LNewArray(MNewArray *)

Eliminate argument.

@@ +189,5 @@
> +  public:
> +    LIR_HEADER(NewArray);
> +
> +    LNewArray(MNewArray *)
> +        : LNewArrayBase(vm)

nit: 2-space indentation before :

@@ +191,5 @@
> +
> +    LNewArray(MNewArray *)
> +        : LNewArrayBase(vm)
> +    {
> +    }

nit: { }

@@ +194,5 @@
> +    {
> +    }
> +
> +    MNewArray *mir() const {
> +        return static_cast<MNewArray *>(mir_);

nit: return mir_->toNewArray();

@@ +197,5 @@
> +    MNewArray *mir() const {
> +        return static_cast<MNewArray *>(mir_);
> +    }
> +
> +    static const VMFunction vm;

If vm is a static const, why is it initializing the LVMCallInstructionHelper?
What is the justification for a common, global VMFunction?

::: js/src/ion/LinearScan.cpp
@@ +481,5 @@
>          // point of definition, if found.
>          for (LInstructionReverseIterator ins = block->rbegin(); ins != block->rend(); ins++) {
>              // Calls may clobber registers, so force a spill and reload around the callsite.
> +            if (ins->isCall()) {
> +                for (AnyRegisterIterator iter; iter.more(); iter++)

Same performance degradation as in Greedy.

::: js/src/ion/Lowering.cpp
@@ +88,5 @@
>      return add(new LGoto(ins->target()));
>  }
>  
>  bool
> +LIRGenerator::visitNewArray(MNewArray *mir)

The prototype in the header calls the variable "ins". It should be "ins" here as well.

@@ +90,5 @@
>  
>  bool
> +LIRGenerator::visitNewArray(MNewArray *mir)
> +{
> +    LNewArray *lir = new LNewArray(mir);

It is unidiomatic for MIR to be explicitly passed to LIR; generally this is handled be defineFoo() functions.
In fact, defineVMReturn() *already* calls setMir()!

::: js/src/ion/MIR.h
@@ +829,5 @@
>  
> +class MNewArray : public MAryInstruction<0>
> +{
> +    // Number of space to allocate for the array.
> +    uint32 count_;

This is called "size" in the IonBuilder. Could we call it "count" there as well?
"size" tends to imply a byte length, whereas "count" in this context would mean "number of slots".

@@ +840,5 @@
> +    MNewArray(uint32 count, types::TypeObject *type)
> +        : count_(count), type_(type)
> +    {
> +        // Do not set it as idempotent to resume after the allocation in case of
> +        // issues.

Doesn't need comment -- memory allocation is known to be fallible.

@@ +842,5 @@
> +    {
> +        // Do not set it as idempotent to resume after the allocation in case of
> +        // issues.
> +
> +        // see interp::stubs::NewInitArray.

Doesn't need comment -- arrays are known to be objects.

::: js/src/ion/shared/Assembler-shared.h
@@ +277,5 @@
>      };
>  
> +    // Determine how to check the ReturnReg of cdecl to determine if a failure
> +    // happened during the C function execution.
> +    enum AssertType {

nit: "ExceptionType" would be more accurate, I think. Or "FallibleType", maybe. We tend to use "Assert" to mean a hard failure, such as JS_ASSERT().

@@ +295,4 @@
>          ReturnBool,
> +        // Return a pointer such as JSObject*.
> +        ReturnPointer,
> +        // Return a Value.

nit: The comments above ReturnType options in the above code should be removed -- they are obvious.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +767,5 @@
>  
>      return true;
>  }
>  
> +const VMFunction LNewArray::vm = {

Is this a valid way to construct structs? I'm reasonably certain that my version of G++ will complain, potentially mentioning something about c++0x. Have you verified that this works across compilers?

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +164,5 @@
> +    Register ret = ReturnReg;
> +    const LDefinition::Policy policy = LDefinition::PRESET;
> +
> +    switch (DefType)
> +    {

nit: { on same line.

@@ +179,5 @@
> +#elif defined(JS_PUNBOX64)
> +        JS_ASSERT(Defs == 1);
> +        type = LDefinition::BOX;
> +        ret = JSCReturnReg;
> +        goto oneDef;

Code duplication is preferred over a goto, especially when it mimics the NUNBOX32 case. The branches are distinct.

::: js/src/ion/x86/Architecture-x86.h
@@ +122,3 @@
>  
>      // Registers returned from a JS -> C call.
>      static const uint32 JSCCallMask =

Maybe we could use "Cpp" instead of "C"?
That would also help "generateCWrapper()", which looks as if it should be pronounced with five syllables instead of six.
Comment 16 David Anderson [:dvander] 2011-12-01 16:18:50 PST
> > +    // Do not use the oracle here because this type object will stay the same for
> > +    // each (script, pc, kind = Array).
> > +    TypeObject *type = TypeScript::InitObject(cx, script, pc, JSProto_Array);
> 
> I don't understand the comment. Why would using an oracle be *wrong*? How
> does this behave without -n?

It's not wrong, it's just conceptually weird. TI is now part of the VM and this code is necessary for initializing objects, even if TI is disabled (you get a generic TypeObject back). So this code is fine.

> Maybe we could use "Cpp" instead of "C"?
> That would also help "generateCWrapper()", which looks as if it should be
> pronounced with five syllables instead of six.

Agree on this just because I think of "JavaScriptCore" :)
Comment 17 Nicolas B. Pierron [:nbp] 2011-12-01 17:43:22 PST
(In reply to Sean Stangl from comment #15)
> Comment on attachment 578341 [details] [diff] [review] [diff] [details] [review]
> Handle LNewArray bytecode instruction.
> 
> Review of attachment 578341 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Rushed comments at the end, since I was informed that this patch is now
> obsolete.
> 
> ::: js/src/ion/GreedyAllocator.cpp
> @@ +590,2 @@
> >  {
> > +    for (AnyRegisterIterator iter; iter.more(); iter++) {
> 
> This will evict more registers than necessary for JS calls. If an
> instruction isCall(), it should remember the appropriate register mask to
> use. This is a performance degradation for no benefit.

This code has come back to its previous state because LSRA may restore registers used as definition of the instruction, and thus erase the return value of the call.

> ::: js/src/ion/IonBuilder.cpp
> @@ +2253,5 @@
> > +    using namespace types;
> > +
> > +    // Do not use the oracle here because this type object will stay the same for
> > +    // each (script, pc, kind = Array).
> > +    TypeObject *type = TypeScript::InitObject(cx, script, pc, JSProto_Array);
> 
> I don't understand the comment. Why would using an oracle be *wrong*? How
> does this behave without -n?

The object created by NewArray function needs a type object, using the oracle implies getting the TypeSet of the return value, pick one TypeObject from it and give it to the NewArray Mir.

The thing is that this work with as soon as the oracle is initialized, but it won't work before, which cause compilation failures.

The TypeObject returned here (AFAIU) would always be the same for this Bytecode (scrip, pc).

> ::: js/src/ion/IonRegisters.h
> @@ +213,5 @@
> >      }
> >      static inline TypedRegisterSet Not(const TypedRegisterSet &in) {
> >          return TypedRegisterSet(~in.bits_ & T::Codes::AllocatableMask);
> >      }
> > +    static inline TypedRegisterSet VolatileNot(const TypedRegisterSet &in) {
> 
> This is a bit fishy. Maybe the mask to use should be part of the template?

template ?  Is VolatileAndNot a better name for this one ?

> ::: js/src/ion/LIR-Common.h
> @@ +183,5 @@
> >      }
> >  };
> >  
> > +typedef LVMCallInstructionHelper<VMFunction::ReturnPointer, 1, 0, 0> LNewArrayBase;
> > +class LNewArray : public LNewArrayBase
> 
> Can we remove the typedef and have the ":" on a newline with a 2-space
> indentation, if it's breaking the 90-char limit?

The thing is, if we don't define it before the class, we would define it inside the class to initialize the parent class.  So I found easier to add a typedef before than duplication the template instance.

> @@ +197,5 @@
> > +    MNewArray *mir() const {
> > +        return static_cast<MNewArray *>(mir_);
> > +    }
> > +
> > +    static const VMFunction vm;
> 
> If vm is a static const, why is it initializing the LVMCallInstructionHelper?
> What is the justification for a common, global VMFunction?

The issue is that we have multiple source of identical information.  In case of changes you would prefer to have an HARD failure with an assertion raised during the compilation than a weird error of register allocation.

The purpose of the  LVMCallInstructionHelper  is to assert as soon as possible that the type used inside defineVMFunction is identical to the type of the VMFunction. (and also again to serve spillRegs due LSRA issue).

> ::: js/src/ion/LinearScan.cpp
> @@ +481,5 @@
> >          // point of definition, if found.
> >          for (LInstructionReverseIterator ins = block->rbegin(); ins != block->rend(); ins++) {
> >              // Calls may clobber registers, so force a spill and reload around the callsite.
> > +            if (ins->isCall()) {
> > +                for (AnyRegisterIterator iter; iter.more(); iter++)
> 
> Same performance degradation as in Greedy.

Is there a way to iterate over the set of register allocated by the RA ?

> @@ +840,5 @@
> > +    MNewArray(uint32 count, types::TypeObject *type)
> > +        : count_(count), type_(type)
> > +    {
> > +        // Do not set it as idempotent to resume after the allocation in case of
> > +        // issues.
> 
> Doesn't need comment -- memory allocation is known to be fallible.

I agree, allocations are fallible but it is also idempotent since we have a GC and that memory management is absent from JavaScript.  The only side effect done by this function is not visible into JavaScript.  So it is legitimate to declare this operation as Idempotent.

> @@ +842,5 @@
> > +    {
> > +        // Do not set it as idempotent to resume after the allocation in case of
> > +        // issues.
> > +
> > +        // see interp::stubs::NewInitArray.
> 
> Doesn't need comment -- arrays are known to be objects.

I think this is necessary because the function may have returned a Value instead of a pointer.  But you are right, I will replace this comment by an assertion inside defineVMReturn.

> ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
> @@ +767,5 @@
> >  
> >      return true;
> >  }
> >  
> > +const VMFunction LNewArray::vm = {
> 
> Is this a valid way to construct structs? I'm reasonably certain that my
> version of G++ will complain, potentially mentioning something about c++0x.
> Have you verified that this works across compilers?

This is valid and also the only way to statically initialized a constant structure.  I would have prefer to use C99 convention where you can name the field, but this is not valid C++.

> @@ +179,5 @@
> > +#elif defined(JS_PUNBOX64)
> > +        JS_ASSERT(Defs == 1);
> > +        type = LDefinition::BOX;
> > +        ret = JSCReturnReg;
> > +        goto oneDef;
> 
> Code duplication is preferred over a goto, especially when it mimics the
> NUNBOX32 case. The branches are distinct.

What ? … ok I removed the goto but I also avoid the duplication (*).

> ::: js/src/ion/x86/Architecture-x86.h
> @@ +122,3 @@
> >  
> >      // Registers returned from a JS -> C call.
> >      static const uint32 JSCCallMask =
> 
> Maybe we could use "Cpp" instead of "C"?
> That would also help "generateCWrapper()", which looks as if it should be
> pronounced with five syllables instead of six.

So you suggest to add 2 additional syllables because it fits your tongue Prediction algorithm ?  Why not removing the "C" ?  After all, we only have one kind of wrapper for now.

*: http://en.wikipedia.org/wiki/Duplicate_code
Comment 18 Nicolas B. Pierron [:nbp] 2011-12-02 11:27:41 PST
Created attachment 578661 [details] [diff] [review]
Handle LNewArray bytecode instruction.
Comment 19 Nicolas B. Pierron [:nbp] 2011-12-02 11:33:23 PST
Comment on attachment 578341 [details] [diff] [review]
Handle LNewArray bytecode instruction.

Added in the new patch:
- Move the interp::NewArray function into this patch.
- Add LDefinition ctor(vreg, type, allocation) to factor defineVMReturn.
- Improve comments
- Assert MIR Types correspond to VMFunction return type.
- Add an abort message when the compilation is failing (useful for checking)
- Apply nits.
Comment 20 Nicolas B. Pierron [:nbp] 2011-12-02 11:40:11 PST
Remove dependency on bug 692838 because this patch now includes the necessary modification of the previous one.
Comment 21 Sean Stangl [:sstangl] 2011-12-02 15:28:26 PST
(In reply to Nicolas B. Pierron [:pierron] from comment #17)
> template ?  Is VolatileAndNot a better name for this one ?

It is just strange for VolatileMask to be arbitrarily used. RegisterSets implicitly operate on AllocatableRegs -- if a RegisterSet needs to operate on VolatileRegs, then that should be a property of the set, not something  arbitrarily performed by a function that is common to all RegisterSets.

> The issue is that we have multiple source of identical information.  In case
> of changes you would prefer to have an HARD failure with an assertion raised
> during the compilation than a weird error of register allocation.

I still don't understand how the VMFunction is being used here. If it is static and const, it is pre-set at compilation-time, and its value is class-wide -- what is the use of asserting based on it? 'vm' is not even initialized to some value.

I think 'vm' can be removed.

> Is there a way to iterate over the set of register allocated by the RA ?

Yes -- this is what the register allocator does currently. Adding a bogus interval causes the register to be written to stack only if the register was already used in the first place.

> I agree, allocations are fallible but it is also idempotent since we have a
> GC and that memory management is absent from JavaScript.  The only side
> effect done by this function is not visible into JavaScript.  So it is
> legitimate to declare this operation as Idempotent.

Idempotent instructions are those which have no side-effects. Any instruction that touches memory in a way that may be seen by other instructions has a side-effect. MNewArray allocates an array in memory: therefore MNewArray cannot be idempotent.

> This is valid and also the only way to statically initialized a constant
> structure.  I would have prefer to use C99 convention where you can name the
> field, but this is not valid C++.

I tried it locally, and GCC 4.6.2 does indeed warn.

> What ? … ok I removed the goto but I also avoid the duplication (*).

As discussed in person, please keep the branches separate. There is a saying from my university: "premature optimization is the root of all evil". The two branches really are distinct, and although they may happen to have similar implementations on one platform, they should be kept separate. We will not notice a difference in runtime either way, and not interleaving the branches is more obvious.

Using an integer flag is not much different from using a goto. It may even be worse!

> So you suggest to add 2 additional syllables because it fits your tongue
> Prediction algorithm ?  Why not removing the "C" ?  After all, we only have
> one kind of wrapper for now.

Yes :)

Well, the wrappers are for C functions, and I think it's nicer to err on the side of verbosity. If we called them "wrappers", the wrappee wouldn't be obvious, but if we called them "CWrappers", intuition kicks in.

The name "CWrapper" is fine -- except that since "W" doesn't make a strong sound, as written it looks very close to an expletive in the English language. We already have "namespace WTF", but at least we can blame Apple for that one :)

I don't have a strong enough opinion to hold up this patch for naming something "CWrapper", but "CppWrapper" is only a slight delta away.
Comment 22 Nicolas B. Pierron [:nbp] 2011-12-03 15:23:50 PST
Created attachment 578864 [details] [diff] [review]
Handle LNewArray bytecode instruction.

- Remove Idemptotent comment on NewArray.
- Move VMFunction to the interpreter.
- rename generateCWrapper to generateVMWrapper.
- Use const reference instead of pointer for VMFunctions.
- Remove js::interp namespace.
Comment 23 Sean Stangl [:sstangl] 2011-12-05 14:02:59 PST
Comment on attachment 578864 [details] [diff] [review]
Handle LNewArray bytecode instruction.

Review of attachment 578864 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonBuilder.cpp
@@ +2252,5 @@
> +{
> +    using namespace types;
> +
> +    // This type object will stay the same for each couple (script, pc).  We do
> +    // not use the type oracle here because the type oracle need to see a few

type oracle needs

@@ +2253,5 @@
> +    using namespace types;
> +
> +    // This type object will stay the same for each couple (script, pc).  We do
> +    // not use the type oracle here because the type oracle need to see a few
> +    // runs before giving us a typeset containing only the result of InitObject

of the InitObject

::: js/src/ion/IonCode.h
@@ +42,5 @@
>  #ifndef jsion_coderef_h__
>  #define jsion_coderef_h__
>  
>  #include "jscell.h"
> +#include "jsinterp.h"

This feels strange, but is fine for the time being.

::: js/src/ion/IonLIR.h
@@ +828,5 @@
> +            JS_ASSERT(Defs == 1);
> +            return Registers::JSCCallMask;
> +          case VMFunction::ReturnNothing:
> +            return 0;
> +        }

default:
  JS_NOT_REACHED("Unknown DefType.");
  return 0;

::: js/src/ion/LIR-Common.h
@@ +197,5 @@
> +        return mir_->toNewArray();
> +    }
> +
> +    static const VMFunction &function() {
> +        return NewInitArrayVMFun;

This is a lot nicer!

@@ +220,5 @@
>  };
>  
>  // Generates a polymorphic callsite, wherein the function being called is
>  // unknown and anticipated to vary.
> +class LCallGeneric : public LCallInstructionHelper<BOX_PIECES , 1, 2>

Does this need changes to ensure that spillRegs() for LCallGeneric does the same thing as before?

::: js/src/ion/MIR.h
@@ +74,5 @@
>      _(InWorklist)                                                               \
>      _(EmittedAtUses)                                                            \
>      _(LoopInvariant)                                                            \
>      _(Commutative)                                                              \
> +    _(Idempotent)    /* The instruction has no C++ side-effects. */             \

The comment wasn't inaccurate before, but the new comment is also not wrong. I would prefer the previous version, since the new one draws a distinction between "JS" and "C++" side-effects which isn't well-defined.

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +174,5 @@
> +        if (getVirtualRegister() >= MAX_VIRTUAL_REGISTERS)
> +            return false;
> +#elif defined(JS_PUNBOX64)
> +        lir->setDef(0, LDefinition(vreg, type, LGeneralReg(JSCReturnReg)));
> +#endif

200 OK

@@ +185,5 @@
> +        type = LDefinition::OBJECT;
> +        lir->setDef(0, LDefinition(vreg, type, LGeneralReg(ReturnReg)));
> +        break;
> +      case VMFunction::ReturnNothing:
> +        return false;

default:
  JS_NOT_REACHED( ... );
  return false;

::: js/src/ion/shared/MoveEmitter-x86-shared.cpp
@@ +195,5 @@
>          masm.mov(toOperand(from), to.reg());
>      } else {
>          // Memory to memory gpr move.
>          Register reg = tempReg();
> +        // Damn, we should choose the tempReg intellignetly instead of

Kindly without "Damn, we should". :)

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +494,5 @@
> +        regs = GeneralRegisterSet::VolatileNot(GeneralRegisterSet(Registers::JSCCallMask));
> +        break;
> +      case VMFunction::ReturnValue:
> +        regs = GeneralRegisterSet::VolatileNot(GeneralRegisterSet(Registers::JSCallMask));
> +        break;

default:
  JS_NOT_REACHED( ... );

::: js/src/jsinterp.cpp
@@ +4801,5 @@
>          goto error;
> +
> +    JSObject *obj = NewInitArray(cx, count, type);
> +    if (!obj)
> +        goto error;

It looks like we can ignore "type" in the error path because it's a GCThing. Is this right?

@@ +5906,5 @@
>  }
> +
> +JSObject*
> +js::NewInitArray(JSContext *cx, uint32 count, types::TypeObject *type)
> +{

It looks like this code is assuming that the TypeObject seen by the interpreter and the TypeObject seen by the JIT are the same -- that is, that the TypeObject cannot change.

It would be nice if we could assert that without going through another allocation.

::: js/src/jsinterp.h
@@ +368,5 @@
> +        FallibleNone,
> +        // false for failure.
> +        FallibleBool,
> +        // NULL for failure.
> +        FalliblePointer

Is this really necessary? It looks like we could get away with boolean "Fallible" and "Infallible" options, and then infer the rest from the ReturnType.

@@ +403,5 @@
> +
> +JSObject*
> +NewInitArray(JSContext *cx, uint32 count, types::TypeObject *type);
> +
> +static const VMFunction NewInitArrayVMFun = {

Since this is now in a header file, "static" may not be as useful.

@@ +405,5 @@
> +NewInitArray(JSContext *cx, uint32 count, types::TypeObject *type);
> +
> +static const VMFunction NewInitArrayVMFun = {
> +    JS_FUNC_TO_DATA_PTR(void *, NewInitArray),
> +    2,

This constant bothers me, but I don't have any great solutions. Constants like this really should be named.
Comment 24 David Anderson [:dvander] 2011-12-05 14:31:55 PST
> ::: js/src/ion/IonCode.h
> @@ +42,5 @@
> >  #ifndef jsion_coderef_h__
> >  #define jsion_coderef_h__
> >  
> >  #include "jscell.h"
> > +#include "jsinterp.h"
> 
> This feels strange, but is fine for the time being.

Hrm... IonCode.h is supposed to be a very lightweight include for the GC. Can we not include jsinterp.h here?

> > +static const VMFunction NewInitArrayVMFun = {
> > +    JS_FUNC_TO_DATA_PTR(void *, NewInitArray),
> > +    2,
> 
> This constant bothers me, but I don't have any great solutions. Constants
> like this really should be named.

If it's an argument count, I think it's fine, though if there's template shenanigans to get it automatically, even better.
Comment 25 Nicolas B. Pierron [:nbp] 2011-12-05 15:02:16 PST
(In reply to Sean Stangl from comment #23)
> Comment on attachment 578864 [details] [diff] [review] [diff] [details] [review]
> Handle LNewArray bytecode instruction.
> 
> Review of attachment 578864 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> @@ +220,5 @@
> >  };
> >  
> >  // Generates a polymorphic callsite, wherein the function being called is
> >  // unknown and anticipated to vary.
> > +class LCallGeneric : public LCallInstructionHelper<BOX_PIECES , 1, 2>
> 
> Does this need changes to ensure that spillRegs() for LCallGeneric does the
> same thing as before?

I restored the spillRegs functions to only spill registers which are not defined by the instruction to avoid issues with the LSRA.  So this should work as before without more changes.

> ::: js/src/ion/MIR.h
> @@ +74,5 @@
> >      _(InWorklist)                                                               \
> >      _(EmittedAtUses)                                                            \
> >      _(LoopInvariant)                                                            \
> >      _(Commutative)                                                              \
> > +    _(Idempotent)    /* The instruction has no C++ side-effects. */             \
> 
> The comment wasn't inaccurate before, but the new comment is also not wrong.
> I would prefer the previous version, since the new one draws a distinction
> between "JS" and "C++" side-effects which isn't well-defined.

Still, this comment is not clear, and it should be fixed in the future.

> ::: js/src/ion/shared/MoveEmitter-x86-shared.cpp
> @@ +195,5 @@
> >          masm.mov(toOperand(from), to.reg());
> >      } else {
> >          // Memory to memory gpr move.
> >          Register reg = tempReg();
> > +        // Damn, we should choose the tempReg intellignetly instead of
> 
> Kindly without "Damn, we should". :)

Damn, I forgot this one. :)

> ::: js/src/jsinterp.cpp
> @@ +4801,5 @@
> >          goto error;
> > +
> > +    JSObject *obj = NewInitArray(cx, count, type);
> > +    if (!obj)
> > +        goto error;
> 
> It looks like we can ignore "type" in the error path because it's a GCThing.
> Is this right?

Nice catch … After looking at the code from JSOP_NEWOBJECT, assuming that it is correct, we do not need to manipulate the TypeObject in case of error.  So luckily this code is correct.

> @@ +5906,5 @@
> >  }
> > +
> > +JSObject*
> > +js::NewInitArray(JSContext *cx, uint32 count, types::TypeObject *type)
> > +{
> 
> It looks like this code is assuming that the TypeObject seen by the
> interpreter and the TypeObject seen by the JIT are the same -- that is, that
> the TypeObject cannot change.
> 
> It would be nice if we could assert that without going through another
> allocation.

When the type analysis is enabled, we may assert that the we do not have more than one object inside the typeset for the return value.  But I don't know how costly this is.

> ::: js/src/jsinterp.h
> @@ +368,5 @@
> > +        FallibleNone,
> > +        // false for failure.
> > +        FallibleBool,
> > +        // NULL for failure.
> > +        FalliblePointer
> 
> Is this really necessary? It looks like we could get away with boolean
> "Fallible" and "Infallible" options, and then infer the rest from the
> ReturnType.

I think it is necessary for 2 reasons:

1/ It separates the part which concern the C function and the part of the wrapper.  Otherwise the logic started to be tricky with the outParam.

2/ We may want to have unfallible functions which are returning boolean (LCompareV::function).

> @@ +405,5 @@
> > +NewInitArray(JSContext *cx, uint32 count, types::TypeObject *type);
> > +
> > +static const VMFunction NewInitArrayVMFun = {
> > +    JS_FUNC_TO_DATA_PTR(void *, NewInitArray),
> > +    2,
> 
> This constant bothers me, but I don't have any great solutions. Constants
> like this really should be named.

I added the comment /* count, type */ behind it, to make explicit what we are counting.

Note You need to log in before you can comment on or make changes to this bug.