1065996 - Clean-up: IonMonkey should only inherit the data part of type policies.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: main.cc

Currently a large number of instructions have a typePolicy function which are doing the same thing.

    TypePolicy *typePolicy() {
        return this;
    }

These function are added manually and sometimes forgotten.  They are used to return a pointer to the class which know how to apply the types on the current instruction.  As the type policies are virtual, for each inheritance of a TypePolicy, we inherit a pointer to the vtable of the TypePolicy.

The inheritance only useful for one thing, which is to inherit the specialize_ fields of the TypePolicy.

What I am suggesting is to divide the TypePolicy in 2, by separating the data from the methods.  Then we still want to keep the inheritance as an aggregation of data, but not methods.  And keep the typePolicy function as a way to dispatch to a static instance of the TypePolicy.

Basically, in the small test case attached to this bug, the old model prints the following sizes:

> MFoo = 24
> MBar = 16
> MQux = 8

and the new one prints:

> MFoo = 16
> MBar = 8
> MQux = 8

In addition, with this model we can automatically insert a typePolicy function for all MIR instructions.  This means that we no longer forget the typePolicy function.  The only side effect is that we need a static instance for each template instantiation, which implies that we would have to enumerate all TypePolicy instances.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8487851 [details]
main.cc

(Sorry for the delay. I often overlook the feedback requests at the top of the request queue.)

Looks good. We'll have to see what the final patch looks like, but most instructions have type policies and it's nice that they are a bit smaller now :)

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Split data / methods of TypePolicy classes.

I verified with gdb, this indeed save 8 bytes per classes having a
TypePolicy.

The diff is quite large but the same modification is applied applied to all
MIR Instructions:
 - typePolicy functions are now defined for all classes in TypePolicy.cpp
 - Inheritance are switched to inherit from the ::Data part of the TypePolicy.
 - Type policies now have a Data structure.
 - The Data structure defines |thisTypePolicy|, and instanced of
   these functions are defined in TypePolicy.cpp.  C++ lookup rules are used
   to provide default values.
 - The same thing is done for the specialization_ fields of the Data
   structure.  This is needed since the adjustInputs function is no longer
   part of the instruction.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8490875 [details] [diff] [review]
Split data / methods of TypePolicy classes.

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

Cool, r=me with comments addressed.

::: js/src/jit/MIR.h
@@ +855,5 @@
> +
> +    // Instructions needing to hook into type analysis should return a
> +    // TypePolicy.
> +    virtual TypePolicy *typePolicy() = 0;
> +    virtual MIRType &typePolicySpecialization() = 0;

Why does it return a reference? I don't see any writes to it. It might be safer to return by value.

@@ -4626,3 @@
>      MIRType specialization() const {
>          return specialization_;
>      }

Can we remove this method because it's also inherited from ArithPolicy::Data?

@@ +4808,2 @@
>      {
> +        specialization_ = powerType;

If we add an |explicit TypeSpecializationData(MIRType specialization)| constructor to TypeSpecializationData, can we do PowPolicy::Data(powerType)? Might be nice to force initialization of specialization_...

::: js/src/jit/TypePolicy.h
@@ +35,5 @@
> +  protected:
> +    // Specifies three levels of specialization:
> +    //  - < Value. This input is expected and required.
> +    //  - == Any. Inputs are probably primitive.
> +    //  - == None. This op should not be specialized.

Nit: MIRType_Any was removed in bug 782077.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> @@ -4626,3 @@
> >      MIRType specialization() const {
> >          return specialization_;
> >      }
> 
> Can we remove this method because it's also inherited from ArithPolicy::Data?

Good point, I will do that.

> @@ +4808,2 @@
> >      {
> > +        specialization_ = powerType;
> 
> If we add an |explicit TypeSpecializationData(MIRType specialization)|
> constructor to TypeSpecializationData, can we do PowPolicy::Data(powerType)?
> Might be nice to force initialization of specialization_...

I will try that as part of a follow-up patch, as the current one is not supposed to change the behavior.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> ::: js/src/jit/MIR.h
> @@ +855,5 @@
> > +
> > +    // Instructions needing to hook into type analysis should return a
> > +    // TypePolicy.
> > +    virtual TypePolicy *typePolicy() = 0;
> > +    virtual MIRType &typePolicySpecialization() = 0;
> 
> Why does it return a reference? I don't see any writes to it. It might be
> safer to return by value.

I don't know why, but I thought that the adjustInputs functions were changing the specialization as it used to be 3 years ago.  Indeed there is no need for that.

Comment 6

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60955e716be3

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/247979f5c8de

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/247979f5c8de