678812 - IonMonkey: Lowering produces a graph containing blocks ending in GOTO with two successors

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Andrew Drake [:adrake]]

Attachment: Test case

See attached test case and CFG post-lowering (courtesy of sstangl's iongraph).

Comment 1

[Andrew Drake [:adrake]]

Attachment: CFG Showing GOTO with two successors

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Unless this is causing problems for LSRA, this might be WONTFIX - lowering tries to change constant conditional to jumps to unconditional jumps. It still emits all paths, but it will never take one of the branches. It's mostly for |while (true)| but I doubt it really matters.

Comment 3

[Andrew Drake [:adrake]]

This causes a couple issues, not just in LSRA, if the dead branch is nontrivial. For one, LSRA still has to allocate on that branch, which can lead to lots of unnecessary split intervals in one of the branches, potentially the live one.

Additionally, if we emit these dead nontrivial basic blocks, we're needlessly jumping around them and throwing away icache locality. This gets worse if these dead blocks happen to get stuck in a loop.

No correctness issues as far as I can tell, but performance can suffer a lot I suspect.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, it won't be as ideal as eliminating the branch in MIR, but it's not any *worse* an emitting a cmp+jcc instead, right?

Comment 5

[Ryan Pearl [:rpearl]]

Bit late on this, but Bug 669796 would solve this issue as a side effect (and I should have time to implement it in the recent future).

Comment 6

[Dan Gohman [:sunfish]]

Bug 669796 is now bug 1029830.

Comment 7

[André Bargull [:anba]]

No longer reproducible, therefore closing as WFM.