1112165 - Recover: SIMD constructors.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

We need to recover SIMD constructors in order to avoid building extra SIMD
objects when this is not expected by a function call or by an object property
(if the store not recovered as well)

To do so, we would have to be able to index the spilled SIMD registers.  Then
the recover instruction will reconstruct the SIMD value based on these values.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Recover SimdBox.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Rename RValueAllocation::Float32 to RValueAllocation::AnyFloat.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8567993 [details] [diff] [review]
Recover SimdBox.

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

Nice!

::: js/src/jit-test/tests/SIMD/recover.js
@@ +1,4 @@
> +if (!this.hasOwnProperty("SIMD"))
> +  quit();
> +
> +function assertEqX4(real, expected, assertFunc) {

this is in lib/simd.js now :)

::: js/src/jit/JitFrameIterator.h
@@ +443,5 @@
>      void writeAllocationValuePayload(const RValueAllocation &a, Value v);
>      void warnUnreadableAllocation();
>  
> +  private:
> +    friend class RSimdBox;

Do we really need the friendship here? Why couldn't floatAllocationPointer be public instead?

::: js/src/jit/Recover.cpp
@@ +1347,5 @@
> +        MOZ_ASSERT_IF(a.mode() == RValueAllocation::FLOAT32_REG,
> +                      a.fpuReg().isFloat32x4());
> +        resultObject = js::CreateSimd<Float32x4>(cx, (const Float32x4::Elem *) raw);
> +        break;
> +      case SimdTypeDescr::TYPE_FLOAT64:

I'd rather have this case not implemented + MOZ_CRASH, instead of having it being less complete than the other two cases (MOZ_ASSERT_IF, and it's untested).  Please remove it here and in IonAnalysis.cpp

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +417,5 @@
>          }
> +
> +        MOZ_ASSERT(payload->isMemory() || payload->isFloatReg());
> +        if (payload->isFloatReg())
> +            alloc = RValueAllocation::Float32(ToFloatRegister(payload));

Guess the next patch renames all Float32...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8567994 [details] [diff] [review]
Rename RValueAllocation::Float32 to RValueAllocation::AnyFloat.

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

Thanks for this renaming.

::: js/src/jit/Snapshots.cpp
@@ +88,5 @@
>  //         DOUBLE_REG [FPU_REG]
>  //           Double value stored in a FPU register.
>  //
> +//         ANY_FLOAT_REG [FPU_REG]
> +//           Any Float value stored in a FPU register.

could you give examples in this comment? "Any Float value (float32, SIMD) ..."

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> ::: js/src/jit/JitFrameIterator.h
> @@ +443,5 @@
> >      void writeAllocationValuePayload(const RValueAllocation &a, Value v);
> >      void warnUnreadableAllocation();
> >  
> > +  private:
> > +    friend class RSimdBox;
> 
> Do we really need the friendship here? Why couldn't floatAllocationPointer
> be public instead?

The reason is that the SnapshotIterator is here to abstract the way we read values.  Adding a JSContext to the read function is not an option.  This method is returning a read-only pointer to the stack which should be manipulated with care, and I prefer to have one tiny "friend" class, than trying to figure out a privacy/security issue later.

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e78f8f5154f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2d2461e064

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/e78f8f5154f2
https://hg.mozilla.org/mozilla-central/rev/0b2d2461e064