Recover: SIMD constructors.

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1112166
(Assignee)

Comment 1

3 years ago
Created attachment 8567993 [details] [diff] [review]
Recover SimdBox.
Attachment #8567993 - Flags: review?(benj)
(Assignee)

Comment 2

3 years ago
Created attachment 8567994 [details] [diff] [review]
Rename RValueAllocation::Float32 to RValueAllocation::AnyFloat.
Attachment #8567994 - Flags: review?(benj)
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...
Attachment #8567993 - Flags: review?(benj) → review+
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) ..."
Attachment #8567994 - Flags: review?(benj) → review+
(Assignee)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78f8f5154f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2d2461e064
https://hg.mozilla.org/mozilla-central/rev/e78f8f5154f2
https://hg.mozilla.org/mozilla-central/rev/0b2d2461e064
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.