943769 - Inline calls to SIMD.float32x4.add and other SIMD functions

Status: RESOLVED DUPLICATE of bug 1112155

(Core :: JavaScript Engine: JIT, defect)

Description

[Ivan Jibaja]

Calls to SIMD.float32x4.add are inlined specially if:

1) Both operands are typed objects of type T where T = either float32x4 or int32x4
2) In that case, insert MLoadVector for each argument (using existing code that skips indirection as a model – detecting MDerivedTypeObject and MVectorTypedObject and shortcircuit)
3) Insert MAddVector with unboxed operands
4) Insert MVectorTypedObject to box result

Comment 1

[haitao]

Attachment: 0001-Use-macros-to-declare-and-define-SIMD-functions.patch

Comment 2

[haitao]

Attachment: 0002-Set-up-SIMD-inlining-infrastructure.patch

Comment 3

[haitao]

Attachment: 0003-Introduce-ToX4-ToX4TypedObject-MIR-and-SIMDInputsPol.patch

Comment 4

[haitao]

Attachment: 0001-Use-macros-to-declare-and-define-SIMD-functions.patch

Rebased with inbound.

Comment 5

[haitao]

Attachment: 0002-Set-up-SIMD-inlining-infrastructure.patch

Rebased with inbound.

Comment 6

[haitao]

Niko, thanks a lot for the review. I rebased the two patches you have reviewed with inbound, could you take another look?

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8355457 [details] [diff] [review]
0003-Introduce-ToX4-ToX4TypedObject-MIR-and-SIMDInputsPol.patch

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

::: js/src/jit/MIR.h
@@ +4148,4 @@
>      }
>  };
>  
> +class MToX4

Please add a comment explaining what this MIR does. The name alone isn't immediately clear.

@@ +4151,5 @@
> +class MToX4
> +  : public MUnaryInstruction
> +{
> +  protected:
> +    MToX4(MDefinition *def, MIRType type)

Please add an assertion regarding the type of `def`. It should always be an object, right?

@@ +4161,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(ToX4)
> +    static MToX4 *New(TempAllocator &alloc, MDefinition *def, MIRType type)
> +    {

Nit: put opening brace on previous line

Comment 8

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8381425 [details] [diff] [review]
0002-Set-up-SIMD-inlining-infrastructure.patch

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

::: js/src/jit/IonTypes.h
@@ +167,5 @@
>      case MIRType_Magic:
>        return JSVAL_TYPE_MAGIC;
> +    case MIRType_Float32x4:
> +    case MIRType_Int32x4:
> +      return JSVAL_TYPE_OBJECT;

It is awkward to have MIR types with no direct JSVAL_TYPE equivalent. I wonder if we can somehow factor this better to prevent this scenario from arising. A quick grep through the source reveals that ValueTypeFromMIRType() is used in a lot of places so it's not readily evident to me, but I think it'd be worth the trouble. This setup worries me. *Probably* a different bug though.

::: js/src/jit/MIR.h
@@ +3997,5 @@
> +    }
> +};
> +
> +#define MSIMD_UNARY_FUNCTION_LIST(V)                                                            \
> +  V(Float32x4Abs, "float32x4.abs", MIRType_Float32x4, MIRType_Float32x4)                        \

Can we somehow derive this information the function lists in SIMD.h? I'd prefer not to repeat it. It feels like we should be able to have one master list with the function and its expected argument types in some form that can be converted as needed. However, looking at SIMD.h, it's not obvious at a quick glance how to avoid the repeated data, so maybe this is just an idle wish.

Comment 9

[haitao]

(In reply to Niko Matsakis [:nmatsakis] from comment #8)
> Comment on attachment 8381425 [details] [diff] [review]
> 0002-Set-up-SIMD-inlining-infrastructure.patch
> 
> Review of attachment 8381425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonTypes.h
> @@ +167,5 @@
> >      case MIRType_Magic:
> >        return JSVAL_TYPE_MAGIC;
> > +    case MIRType_Float32x4:
> > +    case MIRType_Int32x4:
> > +      return JSVAL_TYPE_OBJECT;
> 
> It is awkward to have MIR types with no direct JSVAL_TYPE equivalent. I
> wonder if we can somehow factor this better to prevent this scenario from
> arising. A quick grep through the source reveals that ValueTypeFromMIRType()
> is used in a lot of places so it's not readily evident to me, but I think
> it'd be worth the trouble. This setup worries me. *Probably* a different bug
> though.
> ::: js/src/jit/MIR.h
> @@ +3997,5 @@
> > +    }
> > +};
> > +
> > +#define MSIMD_UNARY_FUNCTION_LIST(V)                                                            \
> > +  V(Float32x4Abs, "float32x4.abs", MIRType_Float32x4, MIRType_Float32x4)                        \
> 
> Can we somehow derive this information the function lists in SIMD.h? I'd
> prefer not to repeat it. It feels like we should be able to have one master
> list with the function and its expected argument types in some form that can
> be converted as needed. However, looking at SIMD.h, it's not obvious at a
> quick glance how to avoid the repeated data, so maybe this is just an idle
> wish.
I have tried to record all the information in SIMD.h, but this will get the macro
item hard to read. Instead currently we could rely on the compiler to detect the
missing function id in MIR Id list as when we add a new item in SIMD.h, the 
corresponding MIR id should be defined, otherwise compiler will complain at 
MCallOptimize.cpp.

Comment 10

[Niko Matsakis [:nmatsakis]]

Try run for first 2 parts: https://tbpl.mozilla.org/?tree=Try&rev=58f7c1fee973

Comment 11

[Niko Matsakis [:nmatsakis]]

Haitao -- it looks like there are some build failures in that try run:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35427108&tree=Try

Comment 12

[haitao]

Attachment: 0002-Set-up-SIMD-inlining-infrastructure.patch

Fixed the style checking failure.

Comment 13

[haitao]

Niko, thanks for trying this. I have fixed the style checking failure and will check the style before submitting patches.

Comment 14

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3194cb1d73f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efaabf97f0c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3194cb1d73f6
https://hg.mozilla.org/mozilla-central/rev/7efaabf97f0c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8381408 [details] [diff] [review]
0001-Use-macros-to-declare-and-define-SIMD-functions.patch

AFAICT, these were checked in.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8384372 [details] [diff] [review]
0002-Set-up-SIMD-inlining-infrastructure.patch

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

::: js/src/jit/MIR.h
@@ +4023,5 @@
> +  V(Int32x4GetSignMask, "int32x4.getSignMask", MIRType_Int32, MIRType_Int32x4)                  \
> +  V(Int32x4GetFlagX, "int32x4.getFlagX", MIRType_Boolean, MIRType_Int32x4)                      \
> +  V(Int32x4GetFlagY, "int32x4.getFlagY", MIRType_Boolean, MIRType_Int32x4)                      \
> +  V(Int32x4GetFlagZ, "int32x4.getFlagZ", MIRType_Boolean, MIRType_Int32x4)                      \
> +  V(Int32x4GetFlagW, "int32x4.getFlagW", MIRType_Boolean, MIRType_Int32x4)

I thought I already mentioned that this was a bad idea to declare so many MIR node when we have so little differences between them?

I clearly remember suggesting that you should use a MVector* over IRC.
Why did this changes landed?

Comment 18

[Nicolas B. Pierron [:nbp]]

All these patches should be backed out because they are not-implementing the Lowering & CodeGen parts.

And as mentioned also to haitao, having generic SIMD instructions that we dispatch based on their number of arguments is not an option!  This is like dispatching on MUnary & MBinary.

Comment 19

[Nicolas B. Pierron [:nbp]]

Please include all the test cases produced by decoder's fuzzer, see all the depend bugs.

Then split your patch to implement each function at a time, from the MIR to the CodeGen.  And with each patch, add the corresponding test case which ensure that the newly inlined function is exercised by a test case.

In the mean time, To let decoder's fuzzer be useful for the rest of the engine, I backed out the second part of this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb9cb300800

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8384372 [details] [diff] [review]
0002-Set-up-SIMD-inlining-infrastructure.patch

Reset checking flag to reflect the backed out patch.

Comment 21

[Niko Matsakis [:nmatsakis]]

nbp -- I discussed with jandem and we disagreed that multiple mir would be superior.

As for checking in with unimplemented code, I do apologize, that's my fault. I should not have given r+, I missed that aspect of the patch. Thanks for backing it out.

Comment 22

[haitao]

nbp and niko, sorry it is my fault to submit the checkin request for the second patch "Set-up-SIMD-inlining-infrastructure.patch" which broke the decoder's fuzzer. I will continue to work on the SIMD stuff from next week and address all your comments and concerns before submitting checkin request.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

This is now being done in bug 1112155, which is a meta bug for all the different operations.