Closed Bug 943769 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1112155

People

(Reporter: ivan, Assigned: haitao.feng)

References

Details

(Whiteboard: [leave-open])

Attachments

(3 files, 3 obsolete files)

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
Blocks: 904913
Attachment #8355453 - Flags: review?(nmatsakis)
Attachment #8355454 - Flags: review?(nmatsakis)
Attachment #8355457 - Flags: review?(nmatsakis)
Attachment #8355453 - Flags: review?(nmatsakis) → review+
Attachment #8355454 - Flags: review?(nmatsakis) → review+
Rebased with inbound.
Attachment #8355453 - Attachment is obsolete: true
Attachment #8381408 - Flags: review?(nmatsakis)
Rebased with inbound.
Attachment #8355454 - Attachment is obsolete: true
Attachment #8381425 - Flags: review?(nmatsakis)
Niko, thanks a lot for the review. I rebased the two patches you have reviewed with inbound, could you take another look?
Attachment #8381408 - Flags: review?(nmatsakis) → review+
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
Attachment #8355457 - Flags: review?(nmatsakis) → review+
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.
Attachment #8381425 - Flags: review?(nmatsakis) → review+
Attachment #8381408 - Flags: review+ → checkin?(nmatsakis)
(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.
Attachment #8381425 - Flags: checkin?(nmatsakis)
Assignee: nobody → haitao.feng
Haitao -- it looks like there are some build failures in that try run:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35427108&tree=Try
Fixed the style checking failure.
Attachment #8381425 - Attachment is obsolete: true
Attachment #8381425 - Flags: checkin?(nmatsakis)
Attachment #8384372 - Flags: review?(nmatsakis)
Attachment #8384372 - Flags: checkin?(nmatsakis)
Niko, thanks for trying this. I have fixed the style checking failure and will check the style before submitting patches.
Attachment #8384372 - Flags: review?(nmatsakis) → review+
Whiteboard: [leave-open]
Comment on attachment 8381408 [details] [diff] [review]
0001-Use-macros-to-declare-and-define-SIMD-functions.patch

AFAICT, these were checked in.
Attachment #8381408 - Flags: checkin?(nmatsakis) → checkin+
Attachment #8384372 - Flags: checkin?(nmatsakis) → checkin+
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?
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.
Attachment #8384372 - Flags: review-
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
Depends on: 980368, 980371, 980400
Comment on attachment 8384372 [details] [diff] [review]
0002-Set-up-SIMD-inlining-infrastructure.patch

Reset checking flag to reflect the backed out patch.
Attachment #8384372 - Flags: checkin+
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.
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.
This is now being done in bug 1112155, which is a meta bug for all the different operations.
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1112155
You need to log in before you can comment on or make changes to this bug.