1050160 - IonMonkey: GVN: DeadIfUnused should not check for resume points.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently GVN do not want to remove instructions which have resume points [1].  The logic is mainly that instructions which have resume points cannot be removed/replaced because they are effectful, but this part of the question should already covered by the isEffectful() condition.

Independently of the instruction, we might want to keep resume points to avoid adding extra register pressure during the register allocation.  When this matters, we are adding MNop which are holding resume points such as we can reduce the register pressure.

So basically, the current condition [1] only matters for keeping the MNop alive across GVN.  We should update this condition such as we can later remove instruction which can be proved to be un-effectful when we are folding them in foldsTo.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/ValueNumbering.cpp#137

Comment 1

[David Moreira [:vash]]

Attachment: patch.diff

Comment 2

[Dan Gohman [:sunfish]]

EliminateDeadCode in IonAnalysis.cpp has similar code. It'd be nice to update it too, perhaps by moving/renaming IsDead so that both can use it.

Comment 3

[David Moreira [:vash]]

Attachment: patch.v2.diff

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8469275 [details] [diff] [review]
patch.v2.diff

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

::: js/src/jit/IonAnalysis.cpp
@@ +517,2 @@
>              {
>                  inst = block->discardAt(inst);

style-nit: Move the open brace to the previous line.

Comment 5

[David Moreira [:vash]]

Attachment: patch.v3.diff

Comment 6

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=42be32e67009

Vash, can you look at what is wrong, and provide a fix for it?
Sorry for the delay.

Comment 7

[David Moreira [:vash]]

Attachment: patch.diff

Seems, that the function has been created by sunfish by a previous commit. I moved the code to IonAnalysis.cpp.

All tests pass.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8515672 [details] [diff] [review]
patch.diff

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

This looks good.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=276053e8f9d4

Comment 9

[Nicolas B. Pierron [:nbp]]

The treeherder link show intermittent failures of the rooting hazards analysis, but this patch should not influence in any way the rooting analysis.

https://hg.mozilla.org/integration/mozilla-inbound/rev/006c4625df01

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/006c4625df01