1136189 - SIMD: inline SIMD constructors even when they have less than 4 arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Currently, we don't inline SIMD constructors if they have less than 4 arguments. However, by spec [0], if any of the missing arguments is undefined, we just put a default value in it, so we can do the same thing at inlining, generating the default constant there directly.

Guillaume, cc'd you on this one, as you asked for it on irc yesterday. If you have any questions, feel free to ask them here or irc (i'm bbouvier there). Cheers!

[0] https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js

Comment 1

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

I introduced a getArg function that creates a default (undefined) value if the arg does not exist.

I removed the two checks about the number of arguments.

Comment 2

[Guillaume Maudoux [:layus]]

Attachment: getArg.diff

This patch is not intended for merging (but is nonetheless valid).

It shows how getArg() could be used in nearly all the inline functions.

pro: Using getArg() simplifies the code by removing most checks on argc()
con: getArg adds a MConstant instruction that is not cleared if we decide not to inline the function. While the useless instruction is removed by some other compiler pass, it adds overhead. 
[french] Faire et défaire, c'est toujours travailler :-) [/french]

How could I improve this idea ?

Comment 3

[Guillaume Maudoux [:layus]]

Of course, attachment 8568847 [details] [diff] [review] (inlineX4.diff) cannot be merged before closing bug 731683 as it depends on the brand new inIon() function :)

Comment 4

[Guillaume Maudoux [:layus]]

Comment on attachment 8568847 [details] [diff] [review]
inlineX4.diff

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

::: js/src/jit/IonBuilder.cpp
@@ +12421,5 @@
> +    if (i < callinfo.argc())
> +       return callinfo.getArg(i);
> +
> +    MConstant *defaultValue = constant(UndefinedValue());
> +    return defaultValue;

nit: return constant(UndefinedValue());

Comment 5

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8569033 [details] [diff] [review]
inlineX4.diff

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

That's a great start!

::: js/src/jit-test/tests/SIMD/inline-missing-arguments.js
@@ +5,5 @@
> +    assertEqX4(SIMD.int32x4(i), [i, 0, 0, 0]);
> +
> +    assertEqX4(SIMD.float32x4(), [NaN, NaN, NaN, NaN]);
> +    assertEqX4(SIMD.float32x4(i), [i, NaN, NaN, NaN]);
> +

Can you add tests for the 2, 3 and 5 arguments cases, please?

::: js/src/jit/IonBuilder.h
@@ +353,5 @@
>      MConstant *constant(const Value &v);
>      MConstant *constantInt(int32_t i);
>  
> +    // Get callinfo arg when it exists or a new undefined value
> +    MDefinition *getArg(CallInfo &callinfo, uint32_t i);

This name is too vague for that purpose. How about calling it getArgOrUndefined?

::: js/src/jit/MCallOptimize.cpp
@@ +2862,5 @@
>      MOZ_ASSERT(&inlineTypedObject->typeDescr() == descr);
>  
>      MSimdValueX4 *values = MSimdValueX4::New(alloc(), simdType,
> +                                             getArg(callInfo, 0), getArg(callInfo, 1),
> +                                             getArg(callInfo, 2), getArg(callInfo, 3));

I think you can even do better than create undefined values here (this will work because of the type analysis phase, which will introduce coercions).  You could create the right constants here directly, with respect to the type of the SIMD operation (for float32x4, create NaN; for int32x4, create 0).  In this case, the renamed getArg chould be a helper in this file (static function, not in IonBuilder::).

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Guillaume Maudoux [:layus] from comment #2)
> Created attachment 8568850 [details] [diff] [review]
> getArg.diff
> 
> This patch is not intended for merging (but is nonetheless valid).
> 
> It shows how getArg() could be used in nearly all the inline functions.
> 
> pro: Using getArg() simplifies the code by removing most checks on argc()
> con: getArg adds a MConstant instruction that is not cleared if we decide
> not to inline the function. While the useless instruction is removed by some
> other compiler pass, it adds overhead. 
> [french] Faire et défaire, c'est toujours travailler :-) [/french]
> 
> How could I improve this idea ?

That's an interesting approach. Regarding the instructions that are added while the function isn't inlined, that can simply fixed by adding the instructions iff we're sure we're inlining (at the last minute). Also, we need to make sure this is correct *and* useful. For correctness, implementing it in a few functions and running the jit-tests will let you know if that's fine to do. If you want to know whether it'd be useful, you can implement it, add a printf statement in the getArg function (for the case one argument is missing) and then run the tracked benchmarks [0] and a browsing session on Firefox. Then, you can observe if your printf shows up sometimes, and if performance benchmarks are affected somehow.

[0] https://github.com/h4writer/arewefastyet

Comment 8

[Guillaume Maudoux [:layus]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> > +    MDefinition *getArg(CallInfo &callinfo, uint32_t i);
> 
> This name is too vague for that purpose. How about calling it
> getArgOrUndefined?

I wanted to reproduce the feeling of get in
 https://dxr.mozilla.org/mozilla-central/source/js/public/CallArgs.h#303 ,
withe example usage in
 https://dxr.mozilla.org/mozilla-central/source/js/src/jsmath.cpp?from=jsmath.cpp#721 .

I wanted to change directly CallInfo::getArg(), but the `current` MBasicBlock is not known to a CallInfo object.
Code like this
|class CallInfo {
|  MDefinition *getArg(uint32_t i) const 
|  {
|     if (i < argc())
|        return args_[i];
|     MConstant* undefined = MConstant::New(args_.alloc, UndefinedValue());
|     fun_->block_->add(undefined)
|     return undefined;
|  }
could work but is hackish (that's an understatement :).



> I think you can even do better than create undefined values here (this will
> work because of the type analysis phase, which will introduce coercions). 
> You could create the right constants here directly, with respect to the type
> of the SIMD operation (for float32x4, create NaN; for int32x4, create 0). 
> In this case, the renamed getArg chould be a helper in this file (static
> function, not in IonBuilder::).

I can certainly do that, but is it worth the growth in code size ?
I would prefer to stick with getArgOrUndefined() if it makes.

Correct me if I am mistaken, but type analysis will inspect each constant and coerce it.
So using undefined in place of NaN (for float32x4) should not add a big overhead.


Finally, I will also try to use the same default value for each missing argument instead of creating four new ones (worst case).

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Guillaume Maudoux [:layus] from comment #8)
> I can certainly do that, but is it worth the growth in code size ?
> I would prefer to stick with getArgOrUndefined() if it makes.
> 
> Correct me if I am mistaken, but type analysis will inspect each constant
> and coerce it.
> So using undefined in place of NaN (for float32x4) should not add a big
> overhead.
> 
> 
> Finally, I will also try to use the same default value for each missing
> argument instead of creating four new ones (worst case).

Yes it is worth it.  The type analysis happens quickly, adding MIR nodes at places where we could have avoided it.  The phase that translates these undefined into the right type is GVN, which happens way later, so that means that in between, we have to handle more MIR nodes, while we could have computed the real value way ahead of time.  In my opinion, anytime you can do something on the behalf that the compiler a few phases before it does it itself, you help the compiler.  So please, help your compiler ;)

Comment 10

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

Completely new approach.

More efficient because only one MConstant is created for all the missing arguments (and only when there is at least one).

I am still puzzled by the fact that |constant(DoubleNaNValue())| translates into a "constant undefined" MIR instruction (according to iongraph), which removes all the benefits of using this hack.

It looks like a bug (missing feature :)) to me.
How am I supposed to create "constant NaN (Float32)" and "constant NaN (Double)" if not by using 
| defVal = constant(JS::DoubleNaNValue());
| defVal->setResultType(scalarType);

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8569518 [details] [diff] [review]
inlineX4.diff

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

Getting better, I'd like to give another look when comments get fixed :)

::: js/src/jit/IonBuilder.cpp
@@ +12415,5 @@
>      return c;
>  }
>  
> +MDefinition *
> +IonBuilder::getArgOrElse(CallInfo &callinfo, uint32_t i, MDefinition *defaultValue)

Hmm, at least one of these two getArg / getArgOrElse functions isn't used in your patch... I'd rather keep the one in CallInfo.

::: js/src/jit/IonBuilder.h
@@ +353,5 @@
>      MConstant *constant(const Value &v);
>      MConstant *constantInt(int32_t i);
>  
> +    // Get callinfo arg when it exists or a new undefined value
> +    MDefinition *getArgOrElse(CallInfo &callinfo, uint32_t i, MDefinition *defaultValue);

nit: here and below, please name it getArgOrDefault. Seeing "else" here makes me think at something related to the control flow of the generated MIR graph, but this isn't related to it at all.

::: js/src/jit/MCallOptimize.cpp
@@ +2851,5 @@
>  
> +    // When there are missing arguments, provide a default value
> +    // containing the coercion of 'undefined' to the right type.
> +    MConstant *defVal = nullptr;
> +    MIRType scalarType = SimdTypeToScalarType(simdType);

This definition can be made inside the if body

@@ +2855,5 @@
> +    MIRType scalarType = SimdTypeToScalarType(simdType);
> +    if (callInfo.argc() < SimdTypeToLength(simdType)) {
> +        if (scalarType == MIRType_Int32) {
> +            defVal = constant(Int32Value(0));
> +        } else { // SimdTypeToScalarType(simdType) == float32 OR double

Don't say it in a comment, MOZ_ASSERT it, please :)

@@ +2857,5 @@
> +        if (scalarType == MIRType_Int32) {
> +            defVal = constant(Int32Value(0));
> +        } else { // SimdTypeToScalarType(simdType) == float32 OR double
> +            defVal = constant(JS::DoubleNaNValue());
> +            // Coerce NaN to |scalarType|.

nit: this comment is false, as using setResultType doesn't do the coercion itself, but will imply it later. In my opinion, it's fine just not having a comment at all, as it's pretty clear what's gonna happen here.

@@ +2860,5 @@
> +            defVal = constant(JS::DoubleNaNValue());
> +            // Coerce NaN to |scalarType|.
> +            defVal->setResultType(scalarType);
> +        }
> +        MOZ_ASSERT(defVal->type() != MIRType_Undefined);

Not sure of the value of this assertion as well...

Comment 12

[Guillaume Maudoux [:layus]]

> > +MDefinition *
> > +IonBuilder::getArgOrElse(CallInfo &callinfo, uint32_t i, MDefinition *defaultValue)
> 
> Hmm, at least one of these two getArg / getArgOrElse functions isn't used in
> your patch... I'd rather keep the one in CallInfo.

of course, I forgot to remove it.

> @@ +2857,5 @@
> > +        if (scalarType == MIRType_Int32) {
> > +            defVal = constant(Int32Value(0));
> > +        } else { // SimdTypeToScalarType(simdType) == float32 OR double
> > +            defVal = constant(JS::DoubleNaNValue());
> > +            // Coerce NaN to |scalarType|.
> 
> nit: this comment is false, as using setResultType doesn't do the coercion
> itself, but will imply it later. In my opinion, it's fine just not having a
> comment at all, as it's pretty clear what's gonna happen here.
> 
> @@ +2860,5 @@
> > +            defVal = constant(JS::DoubleNaNValue());
> > +            // Coerce NaN to |scalarType|.
> > +            defVal->setResultType(scalarType);
> > +        }
> > +        MOZ_ASSERT(defVal->type() != MIRType_Undefined);
> 
> Not sure of the value of this assertion as well...

This is my main problem.
somewhere in the code, my JS::DoubleNaNValue() is turned into an UndefinedValue() because iongraph+genpngs shows :

| 32: constant undefined                                        (Undefined)
| 33: tofloat32 constant32                                      (Float32)
| 34: simdbox4 tofloat3233 tofloat3233 tofloat3233 tofloat3233
| ...

But, if I replace JS::DoubleNaNValue() by Float32Value(6.0) (which is not correct but serves as an example), iongraph shows

| 32: constant 6.0                                         (Float32)
| 33: simdbox4 constant32 constant32 constant32 constant32
| ...

The whole point of comment 9 is to create directly the right default value (here NaN) instead of creating an undefined constant that will be coerced later on.
But it seems impossible to create a float32 NaN MConstant so this is very frustrating.

So my question is : How to create a float32 NaN MConstant that does not get transformed to undefined ?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

I just tried your patch locally and it just creates the correct NaN value directly. Can it be that you forgot to recompile, or clean/recreate the iongraph?

Comment 14

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

Passes the tests and iongraph shows that the right value is created.
This one should be good.

Points to review :
1/ overloaded getArg (python dict.get style) instead of using the name 'getArgOrDefault'.
2/ Added DoubleNaNValue to NamespaceImports.h

Comment 15

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

**** !

Some extra space slipped in the previous version :)

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8570096 [details] [diff] [review]
inlineX4.diff

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

That's looking even better! Please apply the renaming I've suggested a few reviews ago :)

::: js/src/NamespaceImports.h
@@ +52,5 @@
>  
>  using JS::Value;
>  using JS::BooleanValue;
>  using JS::DoubleValue;
> +using JS::DoubleNaNValue;

Doesn't it look overkill to add the using here, while we only need once? See other uses of DoubleNaNValue [0]. Can you instead just use the JS:: prefix in MCallOptimize, please?

[0] https://dxr.mozilla.org/mozilla-central/search?q=DoubleNaNValue&case=true

::: js/src/jit/IonBuilder.h
@@ +1252,5 @@
>          MOZ_ASSERT(i < argc());
>          return args_[i];
>      }
>  
> +    MDefinition *getArg(uint32_t i, MDefinition *defaultValue) const {

I'm sorry, I would really prefer another name here. You get the choice between getArgOrDefault, or getArgWithDefault, or something that says in the name that the second parameter is a default value. While I do love and use Python, I can't see a reason why we would like to mimic its API here. This function is intended for other C++ developers of the JS engine, who may have heard or not about Python before, so let's keep it simple and state in the name that this API acts differently than the one above. The difference is pretty fundamental here: one will crash the program (getArg without default parameter) and the other one uses the default value (sane usage). So it really seems misleading that they have the same name.

::: js/src/jit/MCallOptimize.cpp
@@ +2856,5 @@
> +        MIRType scalarType = SimdTypeToScalarType(simdType);
> +        if (scalarType == MIRType_Int32) {
> +            defVal = constant(Int32Value(0));
> +        } else {
> +            MOZ_ASSERT(scalarType == MIRType_Float32 || scalarType == MIRType_Double);

nit: MOZ_ASSERT(IsFloatingPointType(scalarType));

Comment 17

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

- Removed the problematic inIon call
- Some changes due to merging mozilla-central tip.
- Passes relevant tests : https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d500ecefa5

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8573085 [details] [diff] [review]
inlineX4.diff

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

Cool, thanks for sticking up with this :)

::: js/src/jit/MCallOptimize.cpp
@@ +26,5 @@
>  
>  using JS::TrackedStrategy;
>  using JS::TrackedOutcome;
>  using JS::TrackedTypeSite;
> +using JS::DoubleNaNValue;

nit: can you put it at the top right before TrackedStrategy, please? (and feel free to move TrackedOutcome above TrackedStrategy as well, to enforce alphabetical order)

@@ +2926,5 @@
> +        if (scalarType == MIRType_Int32) {
> +            defVal = constant(Int32Value(0));
> +        } else {
> +            MOZ_ASSERT(IsFloatingPointType(scalarType));
> +            defVal = constant(DoubleNaNValue());

Just thinking about one more little thing: can you add defVal->setResultType(scalarType); here, please? So that we don't introduce an MToFloat32 for float32...

Comment 19

[Guillaume Maudoux [:layus]]

Attachment: inlineX4.diff

Check !

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1a959a603f1

I suppose there is nothing I can do for pushing to mozilla-central, so that's up to you.
How does it work in practice ?

Comment 20

[Guillaume Maudoux [:layus]]

Also, 
| if (callInfo.argc() < SimdTypeToLength(simdType)) {
is a bit redundant here as the function only inlines valuesX4 SIMD constructors.
A shorted version would be
| if (callInfo.argc() < 4) {

??

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8573210 [details] [diff] [review]
inlineX4.diff

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

Cool, thanks! As I put r+ beforehands, you could just upload a new patch with the comment fixed and auto-r+ with a comment saying "carrying forward r+" :)

Now that your patch is r+, you can set the flag "checkin-needed" in the whiteboard, and one of our (awesome) sheriffs will come by and land it on your behalf!

Thanks for this contribution. Would you be interested in other SIMD bugs?

::: js/src/jit/MCallOptimize.cpp
@@ +2920,5 @@
>  
> +    // When there are missing arguments, provide a default value
> +    // containing the coercion of 'undefined' to the right type.
> +    MConstant *defVal = nullptr;
> +    if (callInfo.argc() < SimdTypeToLength(simdType)) {

Keep it like this please, so that we don't have to change anything in this if() when we'll inline other types. (I agree that the code will need to be modified anyways because of the MSimdValueX4 ctor, but let's not prevent future work as much as we can)

Comment 22

[Guillaume Maudoux [:layus]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #21)
> Cool, thanks! As I put r+ beforehands, you could just upload a new patch
> with the comment fixed and auto-r+ with a comment saying "carrying forward
> r+" :)

Nice to know !

> Now that your patch is r+, you can set the flag "checkin-needed" in the
> whiteboard, and one of our (awesome) sheriffs will come by and land it on
> your behalf!

Ok, except that i cannot edit the whiteboard.
I suppose I must at least be the asignee to do that, and a cannot assign the bug myself.
 
> Thanks for this contribution. Would you be interested in other SIMD bugs?

Why not ? But I will first land (or dismiss) the inIon() patch.

Thanks for mentoring me throug the various steps.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca2ba62ff1df

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca2ba62ff1df