1072911 - Scalar Replacement: Add MIRType to MPhi & M*State definitions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

See tests cases from bug 1069307 comment 1.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Scalar Replacement: Add missing MIRTypes.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8495235 [details] [diff] [review]
Scalar Replacement: Add missing MIRTypes.

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

Note: I added the last 2 test cases in the patch as the first test case do an infinite loop and highlight the same assertion.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8495235 [details] [diff] [review]
Scalar Replacement: Add missing MIRTypes.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +310,5 @@
>          // nodes.
>          succState = BlockState::Copy(alloc_, state_);
>          size_t numPreds = succ->numPredecessors();
>          for (size_t slot = 0; slot < state_->numSlots(); slot++) {
> +            MPhi *phi = MPhi::New(alloc_, MIRType_Value);

No need for this change. MPhi has a default resultType of MIRType_Value

@@ +764,5 @@
>          // nodes.
>          succState = BlockState::Copy(alloc_, state_);
>          size_t numPreds = succ->numPredecessors();
>          for (size_t index = 0; index < state_->numElements(); index++) {
> +            MPhi *phi = MPhi::New(alloc_, MIRType_Value);

No need for this change. MPhi has a default resultType of MIRType_Value

Comment 4

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11a4a935eb7d

Comment 5

[Mayank Bansal]

there appears to be a regression (19672 -> 17651) on Octane-Splay on machine 11 on AWFY. 
The regression range is this from AWFY : http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=846faaa68219&tochange=808aa539aad1

Comment 6

[Nicolas B. Pierron [:nbp]]

Hannes, I do not think this can cause any performance impact as the only modifications are on unused objects/arrays.  This issue is caused by the fix in Bug 1007213, and would be resolved by the optimization from Bug 1072188.

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Hannes, I do not think this can cause any performance impact as the only
> modifications are on unused objects/arrays.  This issue is caused by the fix
> in Bug 1007213, and would be resolved by the optimization from Bug 1072188.

Yeah that sound likely. Thanks. I'll keep an eye on bug 1072188. Let's hope it can land soonish so this regression is gone.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11a4a935eb7d