Closed Bug 1050160 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nbp, Assigned: vash)

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8469218 - Flags: review?(nicolas.b.pierron)
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.
Attached patch patch.v2.diff (obsolete) — Splinter Review
Attachment #8469218 - Attachment is obsolete: true
Attachment #8469218 - Flags: review?(nicolas.b.pierron)
Attachment #8469275 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8469275 - Flags: review?(nicolas.b.pierron) → review+
Attached patch patch.v3.diff (obsolete) — Splinter Review
Attachment #8469275 - Attachment is obsolete: true
Attachment #8469330 - Flags: review+
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron) → needinfo?(davidmoreirafr)
Attached patch patch.diffSplinter Review
Seems, that the function has been created by sunfish by a previous commit. I moved the code to IonAnalysis.cpp.

All tests pass.
Attachment #8469330 - Attachment is obsolete: true
Flags: needinfo?(vash)
Attachment #8515672 - Flags: review?(nicolas.b.pierron)
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
Attachment #8515672 - Flags: review?(nicolas.b.pierron) → review+
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
https://hg.mozilla.org/mozilla-central/rev/006c4625df01
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.