1623437 - ARM64: Improve negative zero check for LMulI

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Iain Ireland [:iain]]

Servo had profiles suggesting that Ion is recompiling repeatedly on arm64. jdm and I investigated, and it looks like the primary culprit in his testcase is our handling of integer multiplies producing zero.

For what I assume are good reasons, when an integer multiplication "should" produce negative zero, we bail out to handle it in baseline. (For reasons I haven't tracked down yet, the bailout kind is Bailout_DoubleOutput, which immediately invalidates the IonScript.) For x86/x64, we test to see if the result of the multiply is zero, then use an OOL path to decide whether it was negative zero. For arm64, we don't do that. Instead, we bail out whenever the result of the multiply is zero, negative or not.

Here's a cut down testcase:

var r = 0;
function scale(t) {
  r = r * t;
};

for (var i = 0; i < 10000; i++) { scale(1); }

On arm64, this throws away the IonScript when we execute r = r * t. What's worse, even though we're bailing out with Bailout_DoubleOutput, there's no real double here, so the typeset doesn't change and we make the same mistake next time scale is compiled.

We should fix visitMulI on arm64 so that it doesn't bail out unless the zero is actually negative.

Comment 1

[Jan de Mooij [:jandem]]

Good find.

Longer-term we should really try to catch these bailout -> compile -> bailout loops in debug builds.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #1)

Good find.

Longer-term we should really try to catch these bailout -> compile -> bailout loops in debug builds.

One idea in the past was to checksum the generated code, then record and assert that we did not already produced this code since the last GC.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

One idea in the past was to checksum the generated code, then record and assert that we did not already produced this code since the last GC.

Yeah, with WarpBuilder we can hopefully do this a bit more easily with the WarpSnapshot interface (eventually).

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1623437: ARM64: Check for negative zero in MulI bailout r=nbp

Whenever Ion multiplies two integers on ARM64, if the result is zero, we will not only bail out, but invalidate the script. This is causing repeated invalidations in Servo. This patch fixes the negative zero handling for LMulI on ARM64.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b4897b1963
ARM64: Check for negative zero in MulI bailout r=nbp

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/94b4897b1963