904079 - IonMonkey: MBox(MConstant) emits an LValue for every snapshot that captures it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

MBox(MConstant) is emitted-at-uses and this can cause us to emit an LValue for every snapshot that captures this MBox.

Not only is this a silly performance problem (the value is a constant so these moves and register uses for every snapshot are totally unnecessary), it also breaks Ion's try-catch implementation if there's an LValue between an instruction and the LOsiPoint that follows it. decoder found the testcase I added (the first test in bug 902978 comment 6).

This patch also asserts snapshot uses other than constants are not emitted-at-uses, to avoid similar bugs in the future.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 788962 [details] [diff] [review]
Patch

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

::: js/src/jit/shared/Lowering-shared.cpp
@@ +90,5 @@
>  
> +            // Snapshot operands other than constants should never be
> +            // emitted-at-uses. Try-catch support depends on there being no
> +            // code between an instruction and the LOsiPoint that follows it.
> +            JS_ASSERT_IF(!ins->isConstant(), !ins->isEmittedAtUses());

We should just *never* call ensureDefined below and check that the instruction is either emitted at uses or lowered.
This is not only wrong with Try-cacth, but also with any call.  the LOsiPoint must always directly succeed the instruction which requires a safepoint.

Comment 2

[Jan de Mooij [:jandem]]

Pushed with the ensureDefined call removed and the x64 code updated as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcadb754626

https://tbpl.mozilla.org/?tree=Try&rev=9e008b0e71f5

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dcadb754626