Closed Bug 1046183 Opened 6 years ago Closed 6 years ago

IonMonkey: Move Scalar Replacement after the EliminatePhis phase.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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.
Blocks: 1046197
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@
Attachment #8464752 - Flags: review?(jdemooij) → review+
(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 :(
https://hg.mozilla.org/mozilla-central/rev/1043b94f1969
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.