956299 - IonMonkey: Clean-up: Abstract trivial callVM codegen.

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Nicolas B. Pierron [:nbp]]

At the moment, if we want to add a New MIR node which is calling one VM function, we need to modify at least 8 files (MIR.h, MOpcode.h, LIR-Common.h, LOpcode.h, Lowering.h, Lowering.cpp, CodeGenerator.h, CodeGenerator.cpp).

Doing so is verbose, and add many possible points of failures, where most of the information can be inferred from the VM function prototype.

As MIR and LIR nodes are still based on an enumerated list, and we cannot easily replaced them, so we should declare a list of macro which are used for these VMFunction and there prototypes.  Then we can make a parametrized MIR & LIR, which are used as based classes for the one which would be generated automatically by some Macro.

Ideally, we should be able to reduce the addition of any VM function call to only 1 file which contains a list of names, prototypes and function names such as:

---- VMIROpcode.h ----

typedef bool (*HasInstanceFn)(JSContext *, HandleObject, HandleValue, bool *);

#define VMIR(_) \
  _(HasInstance, js::HasInstance)

---------

I can mentor anybody who want to work on this bug, under the condition that you understand the logic inside VMFunctions.h and that you have already added one or multiples instruction such as the one which are abstracted here.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> we need to modify at least 8 files (MIR.h, MOpcode.h,
> LIR-Common.h, LOpcode.h, Lowering.h, Lowering.cpp, CodeGenerator.h,
> CodeGenerator.cpp).

And of course, to illustrate once more this, I accidentally forgot to mention ParallelSafetyAnalysis.cpp.

Comment 2

[Romain Perier]

I am interested by this bug. I already worked on such an instruction, see https://bugzilla.mozilla.org/show_bug.cgi?id=956051

Comment 3

[Romain Perier]

Attachment: Patch for LIR version 1

I propose to work on this bug in three steps:
1. The LIR and Lowering part (as a first step everything would be parametrized in jit/Lopcodes.h)
2. The MIR part  (as a second the MIR part would be parametrized in jit/Mopcodes.h)
3. Try to merge Lopcodes.h and Mopcodes.h and propose a simple interface

This patch includes a PoC for the first step

Comment 4

[Romain Perier]

The actual interface is the following :

#define LIR_VMFUNCTION_OPCODE_LIST(_)    \
    _(RegExpExec, RegExpExecPrototype, RegExpExecCodeGenArgs, regexp_exec_raw)

Where each arguments are:
- RegExpExec: the name of the instruction (opcode)
- RegExpExecPrototype: a typedef to a function pointer type corresponding to vmfunction prototype
- RegExpExecCodeGenArgs: A list of types/arguments to push during codegeneration (everything is infered with function traits and class trait later). Data are pushed in the same order that arguments in this function pointer.
- regexp_exec_raw: a function pointer to the vmfunction to call throught "callVM"


For the code generation, arguments are the following types:
- MIROperand<N>: Get operand <N> from the MIR , store it inside the LIR node (at <N>) then push it
- MIRObject<N>: Get a data from the MIR node, actually we need to edit this part by hand in the MIR node, it will be generated later
- ValueOperand<N>: Push Value from a LIR Node using "ToValue" function (coming soon, it is not hard to do)

Comment 5

[Romain Perier]

For pushing data from the MIR, I propose the following design :

in the MIR node , add the following nested class trait

class MIRObjectTrait<MIRObject0>
{
  typedef <mytype> type;
};

then add the following templated method:
template <> MIRObjectTrait<MIRObject0>::type get<MIRObject0>() const { /* return  your custom data here */}

----
the codegen just needs to call get<> with the corresponding type during the generation phase, MIRObjectTrait is used to infer the correct Imm allocator.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8378127 [details] [diff] [review]
Patch for LIR version 1

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

::: js/src/jit/CodeGenerator.cpp
@@ +95,5 @@
>      IONCACHE_KIND_LIST(VISIT_CACHE_FUNCTION)
>  #undef VISIT_CACHE_FUNCTION
>  };
>  
> +

nit: Remove extra new-line.

@@ +169,5 @@
> +    typedef ImmGCPtr ImmAllocator;
> +};
> +
> +template<typename T> struct TypeToImm { /* Unexpected type argument for pushing. */};
> +template<> struct TypeToImm<RegExpObject *>: public TypeToImmGCPtr { };

template<> struct TypeToImm<RegExpObject *> { typedef ImmGCPtr ImmAllocator; };

fits on less than 100 character, no need to use an inheritance to work-around the style-guide.

@@ +174,5 @@
> +template<> struct TypeToImm<void *> { typedef ImmPtr ImmAllocator; };
> +template<> struct TypeToImm<int32_t> { typedef Imm32 ImmAllocator; };
> +template<> struct TypeToImm<uintptr_t> { typedef ImmWord ImmAllocator; };
> +
> +// SFINAE 

nit: remove trailing-whitespaces from this patch

@@ +176,5 @@
> +template<> struct TypeToImm<uintptr_t> { typedef ImmWord ImmAllocator; };
> +
> +// SFINAE 
> +template<typename LIR, typename ObjectIndex>
> +static inline void pushMIRObject(CodeGenerator *gen, LIR *lir, typename LIR::template MIRObjectTrait<ObjectIndex>::type)

indent properly the arguments.

@@ +178,5 @@
> +// SFINAE 
> +template<typename LIR, typename ObjectIndex>
> +static inline void pushMIRObject(CodeGenerator *gen, LIR *lir, typename LIR::template MIRObjectTrait<ObjectIndex>::type)
> +{
> +    gen->pushArg(TypeToImm<typename LIR::template MIRObjectTrait<ObjectIndex>::type>::ImmAllocator(lir->mir()->template get<ObjectIndex>()));

Remove useless use of template.

@@ +196,5 @@
> +
> +    void pushOperand(size_t index)
> +    {
> +        if (lir_->getOperand(index)->isConstant())
> +            gen_->pushArg(ImmGCPtr(static_cast<js::gc::Cell *>(lir_->getOperand(index)->toConstant()->toGCThing())));

indent properly.

@@ +202,5 @@
> +            gen_->pushArg(ToRegister(lir_->getOperand(index)));
> +    }
> +
> +  public:
> +    ArgResolver(CodeGenerator *gen, LIR *lir): gen_(gen), lir_(lir) 

remove trailing white space.
Move the init list on the next line, and use '{ }'

@@ +206,5 @@
> +    ArgResolver(CodeGenerator *gen, LIR *lir): gen_(gen), lir_(lir) 
> +    {
> +    }
> +
> +    inline void operator()(const MIRObject0& obj) { pushMIRObject<LIR, MIRObject0>(gen_, lir_); }

Why pushMIRObject is not part of the ArgResolver?

::: js/src/jit/LIR-Common.h
@@ +3378,5 @@
>          return mir_->toStringReplace();
>      }
>  };
>  
> +template<typename fun> struct NumOfOperand { static const unsigned result = 0; };

rename it to NumOfLOperand / LOperandSize.

@@ +3387,5 @@
> +template<> struct NumOfOperand<ValueOperand1> { static const unsigned result = BOX_PIECES; };
> +template<> struct NumOfOperand<ValueOperand2> { static const unsigned result = BOX_PIECES; };
> +
> +template<typename R, typename Last> struct NumOfDefs { static const size_t number = 1; };
> +template<> struct NumOfDefs<bool, Value *> { static const size_t number = BOX_PIECES; };

This specialization should no longer be considered as an out-param.

@@ +3452,5 @@
> +class LIRVMFunction<Defs, void (*)(A1, A2, A3)> : public LCallInstructionHelper<Defs, LIRNumOfOperands<void (*)(A1, A2, A3)>::number, 0>
> +{
> +  public:
> +    template<class Function>
> +    inline void forEachArg(Function fn) const

constYou do not have to specialize the class X number of times, you can provide one specific class specialization, and instanciate forEachArg for each different type of arguments.

@@ +3456,5 @@
> +    inline void forEachArg(Function fn) const
> +    {
> +        fn(A1());
> +        fn(A2());
> +        fn(A3());

The list of arguments is in the wrong order.  The last arguments are supposed to be pushed first and not last.

::: js/src/jit/LOpcodes.h
@@ +13,5 @@
> +using js::jit::MIROperand0;
> +using js::jit::MIROperand1;
> +using js::jit::MIROperand2;
> +
> +typedef bool (*RegExpExecPrototype)(JSContext *, HandleObject, HandleString, Value *);

Respect the same naming convention as for other VM Function, i-e RegExpExecFn

@@ +14,5 @@
> +using js::jit::MIROperand1;
> +using js::jit::MIROperand2;
> +
> +typedef bool (*RegExpExecPrototype)(JSContext *, HandleObject, HandleString, Value *);
> +typedef void (*RegExpExecCodeGenArgs)(MIROperand1, MIROperand0);

Use a shorter suffix, ArgTypes, Types, Kind?

::: js/src/jit/Lowering.cpp
@@ +1944,5 @@
>      return false;
>  }
>  
> +//TODO: Future changes: Add JS_ASSERT (using informations given in Mopcodes.h)
> +//TODO: infer if there is a return value

the style is :TODO:, and every TODO should have its bug number in the comment.
Also MOZ_ASSERT is the new JS_ASSERT.

::: js/src/jit/Lowering.h
@@ +48,5 @@
> +        GetOperand(LIR *lir, LIRGenerator *gen): lir_(lir), gen_(gen) {}
> +
> +        inline void operator()(const MIROperand0 &op) { setOperand(0); }
> +        inline void operator()(const MIROperand1 &op) { setOperand(1); }
> +        inline void operator()(const MIROperand2 &op) { setOperand(2); }

void operator()(const MIRObject0& obj) is missing.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Nicolas B. Pierron [:nbp]]

This bug has been somewhat fixed and worsen by having the yaml files to declare new MIR nodes, and worsen by having CacheIR in the loop, making an even larger pipeline to handle.

While it is now possible to have less modification, we still have a huge list of files that might have to be modified.
I will close this one in the mean time.