1133565 - SIMD: Factor template objects within a compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Each time Baseline sees a SIMD call site, it will create a template object of the right kind.  This is inefficient, as each SIMD operation has its own call site.  We can spare some memory by factoring the template objects.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simd-templates.patch

How does this look? Sorry if I am misusing any function here, I'm fairly new to all this compartment world...

I've looked on the SIMD mandelbrot demo, it prevents creation from ~10 template objects.  All these objects are inlined, so not sure it's a big win, we might want to look at real world SIMD kernels compiled from Emscripten.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8565131 [details] [diff] [review]
simd-templates.patch

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

Nice!

::: js/src/builtin/TypedObject.h
@@ +333,5 @@
>      enum Type {
>          TYPE_INT32 = JS_SIMDTYPEREPR_INT32,
>          TYPE_FLOAT32 = JS_SIMDTYPEREPR_FLOAT32,
> +        TYPE_FLOAT64 = JS_SIMDTYPEREPR_FLOAT64,
> +        LAST_TYPE

This name is a bit confusing; I think it'd be clearer as LAST_TYPE = TYPE_FLOAT64 and then use +1 when you use it. Or call it NUM_TYPES.

::: js/src/jit/JitCompartment.h
@@ +433,5 @@
>      JitCode *stringConcatStub_;
>      JitCode *regExpExecStub_;
>      JitCode *regExpTestStub_;
>  
> +    JSObject *simdTemplateObjects_[SimdTypeDescr::LAST_TYPE];

Use mozilla::Array so that we get bounds checks automatically.

@@ +442,5 @@
>  
>    public:
> +    JSObject *getSimdTemplateObjectFor(JSContext *cx, Handle<SimdTypeDescr*> descr) {
> +        MOZ_ASSERT(descr->type() < SimdTypeDescr::LAST_TYPE);
> +        JSObject **tpl = &simdTemplateObjects_[descr->type()];

I think the array should use ReadBarrieredObject instead of JSObject*, this is necessary for incremental GC. I thought we had to access these pointers during Ion compilation, but we don't so it should be fine.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Factor SIMD templates within a compartment

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8565445 [details] [diff] [review]
Factor SIMD templates within a compartment

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

Looks good!

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8565445 [details] [diff] [review]
Factor SIMD templates within a compartment

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

nit: Remove the comment from MSimdBox::congruentTo.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8565445 [details] [diff] [review]
Factor SIMD templates within a compartment

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

Beautiful.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks! and with the comment removed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/04a909655b92

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04a909655b92