1046183 - IonMonkey: Move Scalar Replacement after the EliminatePhis phase.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently the Scalar Replacement phase is done before the eliminate phis phase, the reason being that the Scalar Replacement phase is adding extra Phis which would have to be removed.

The problem of doing so, is that Scalar Replacement might miss a lot of cases, where the same object is  merged with it-self.

  var o = {a:1}; // (23) MNewObject
  if (…)
    …
  else
    …
  // Phi(23, 23), is not yet removed and
  // prevent the IsObjectEscaped to return false.

Moving Scalar Replacement after the eliminate Phis and running the Eliminates Phis again should solve this issue.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Move Scalar Replacement after the EliminatePhis phase.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8464752 [details] [diff] [review]
Move Scalar Replacement after the EliminatePhis phase.

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

Nice, I wonder if we can handle more cases now in benchmarks :)

::: js/src/jit/ScalarReplacement.cpp
@@ +250,5 @@
>      }
>  
>      if (succ->numPredecessors() > 1) {
> +        // We need to re-compute successorWithPhis as the previous EliminatePhis
> +        // phase might have removed all the Phi from the successor block.

Nit: Phis

@@ +804,5 @@
>  
> +    if (addedPhi) {
> +        // Phis added by Scalar Replacement are only redundant Phis which are
> +        // not directly captured by any resume point but only by the MDefinition
> +        // state. The conservative observability only focus on Phis which are

Nit: s@focus@focuses@

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Nice, I wonder if we can handle more cases now in benchmarks :)

I added the spew in Bug 1046870, and sadly no :(

Comment 4

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1043b94f1969

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/1043b94f1969