1688913 - Follow-up changes for bug 1687441

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

André suggested some performance improvements that are worth addressing.

• Inline IntPtr constants for LCompare
• Avoid two MArrayBufferViewLength for .length (Int32) and bounds check (IntPtr) with a separate IntPtrToInt32 guard
• Related, optimize bounds check hoisting better for trivial i < ta.length case
• See if we can use redefine for no-op MInt32ToIntPtr
• move64 optimization: https://bugzilla.mozilla.org/show_bug.cgi?id=1586662
• Fix AnalyzeLoadUnboxedScalar, although it has no effect with Spectre mitigations on
• Support IntPtr (for MInt32ToIntPtr) in RangeAnalysis::addRangeAssertions.
• Propagate range for MNonNegativeIntPtrToInt32.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 1 - Support int32 constants for Compare_UIntPtr. r?iain!

Change useAnyOrConstant to useAnyOrInt32Constant and use for Compare_UIntPtr.
For IntPtr values, useAnyOrInt32Constant checks a constant is only used for values
in the Int32 range.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 2 - Use useAny for RHS of pointer-size comparison. r?iain!

Drive-by change: for Compare_Symbol and Compare_Object this allows the RHS to be
a memory operand on non-ARM platforms, instead of requiring a register.

Depends on D103482

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 3 - Support range analysis checks for MIRType::IntPtr nodes. r?iain!

Very similar to MIRType::Int32 but use branchPtr instead of branch32 for codegen.

Depends on D103483

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 4 - Use separate NonNegativeIntPtrToInt32 node for ta.length. r?anba!,iain!

This makes GVN and bounds check optimizations work better.

Depends on D103484

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02ffbca866a0
part 1 - Support int32 constants for Compare_UIntPtr. r=iain
https://hg.mozilla.org/integration/autoland/rev/ea00bb31867e
part 2 - Use useAny for RHS of pointer-size comparison. r=iain
https://hg.mozilla.org/integration/autoland/rev/5f1f8088956c
part 3 - Support range analysis checks for MIRType::IntPtr nodes. r=iain
https://hg.mozilla.org/integration/autoland/rev/886fe5870eb6
part 4 - Use separate NonNegativeIntPtrToInt32 node for ta.length. r=anba,iain

Comment 6

[Atila Butkovits]

Backed out for causing build bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a9360c938ce15d9712c628c3252796b82916b855

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cretry%2Cusercancel%2Crunnable&revision=886fe5870eb6cbb610faf04138c8672b3a83646c&searchStr=build&selectedTaskRun=R-zPlGf2T2eYY-ziu6fC-Q.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328472209&repo=autoland&lineNumber=21775

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58cbe2d6f575
part 1 - Support int32 constants for Compare_UIntPtr. r=iain
https://hg.mozilla.org/integration/autoland/rev/1facc48a0706
part 2 - Use useAny for RHS of pointer-size comparison. r=iain
https://hg.mozilla.org/integration/autoland/rev/b86a32fff5d5
part 3 - Support range analysis checks for MIRType::IntPtr nodes. r=iain
https://hg.mozilla.org/integration/autoland/rev/90c909eee4b7
part 4 - Use separate NonNegativeIntPtrToInt32 node for ta.length. r=anba,iain

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/58cbe2d6f575
https://hg.mozilla.org/mozilla-central/rev/1facc48a0706
https://hg.mozilla.org/mozilla-central/rev/b86a32fff5d5
https://hg.mozilla.org/mozilla-central/rev/90c909eee4b7

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 8 - Optimize Int32 <=> IntPtr conversions a bit more. r?anba!

32-bit platforms: alawys use redefine to avoid an unnecessary register move.

64-bit platforms: use redefine for LInt32ToIntPtr when we know input >= 0
(checked in debug builds with LAssertInt32IsIntPtr).

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 9 - Use useAtStart instead of useRegisterAtStart for LInt32ToIntPtr operand. r?anba!

It's easy enough to support memory operands with load32SignExtendToPtr.

Depends on D103754

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 7 - Don't share bundles for MUST_REUSE_INPUT definitions if they don't match. r?iain!

We're already using defineReuseInput for LNonNegativeIntPtrToInt32 and the next patch
uses it for LInt32ToIntPtr too. This exposed an issue on Try: we can't merge those
virtual registers into a single bundle (allocation) because we can get confused about
the stack slots (for example do an 8-byte write for IntPtr to a 4-byte Int32 slot).

This adds some code to disable the tryMergeReusedRegister optimization in this case.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 5 - Clean up safepoint code now that Value slots are only used on 64-bit. r?iain!

With the old Linear Scan register allocator, the list of Value stack slots in
safepoints was used on 32-bit platforms too: the allocator tried hard to spill
Value type and payload slots to adjacent stack slots so that we could trace them
as a single Value. If this failed, the type and payload halves were encoded
separately as "nunbox parts".

With the backtracking allocator we now always use the "nunbox parts" encoding on
32-bit. The Value slots in the safepoints are only used on 64-bit platforms
(addValueSlot is only called on 64-bit). This patch makes this more explicit by
either using "value slots" (64-bit) or "nunbox slots" (32-bit).

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 6 - Use 4 bytes instead of 8 bytes for nunbox half stack slots. r?iain!

The old register allocator allocated 8 bytes for nunbox type or payload. It was
then able to use the same 8-byte slot for spilling the other half and could add
this Value-slot to safepoints as a single unit.

The backtracking allocator, however, doesn't have this optimization so allocating
8 bytes for nunbox type/payload is a waste: we only ever use 4 bytes.

This simplifies the next patch.

Depends on D104363

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 10 - Pass offsetAdjustment to ToAddress for LLoadUnboxedScalar. r?anba!

This isn't observable because offsetAdjustment is always 0 (AnalyzeLoadUnboxedScalar
doesn't know about the Int32 to IntPtr conversion).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74776ac44456
part 5 - Clean up safepoint code now that Value slots are only used on 64-bit. r=iain
https://hg.mozilla.org/integration/autoland/rev/de63db33c09c
part 6 - Use 4 bytes instead of 8 bytes for nunbox half stack slots. r=iain
https://hg.mozilla.org/integration/autoland/rev/4ef58cf45f1f
part 7 - Don't share bundles for MUST_REUSE_INPUT definitions if they don't match. r=iain

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa08364eea3a
part 8 - Optimize Int32 <=> IntPtr conversions a bit more. r=anba
https://hg.mozilla.org/integration/autoland/rev/b072991bff68
part 9 - Use useAtStart instead of useRegisterAtStart for LInt32ToIntPtr operand. r=anba

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/74776ac44456
https://hg.mozilla.org/mozilla-central/rev/de63db33c09c
https://hg.mozilla.org/mozilla-central/rev/4ef58cf45f1f
https://hg.mozilla.org/mozilla-central/rev/aa08364eea3a
https://hg.mozilla.org/mozilla-central/rev/b072991bff68

Comment 18

[Jan de Mooij [:jandem]]

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

• Fix AnalyzeLoadUnboxedScalar, although it has no effect with Spectre mitigations on

I'm not sure what to do with this one: I have a patch to make this work on a local test case, but the optimization doesn't kick in on any jit-tests or Octane (tested by adding a MOZ_CRASH()) with --spectre-mitigations=off.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 10 - Improve ranges of MNonNegativeIntPtrToInt32 and similar instructions. r?anba!,nbp!

As suggested, we can just propagate the range from the input.

Comment 20

[Jan de Mooij [:jandem]]

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

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

• Fix AnalyzeLoadUnboxedScalar, although it has no effect with Spectre mitigations on

I'm not sure what to do with this one: I have a patch to make this work on a local test case, but the optimization doesn't kick in on any jit-tests or Octane (tested by adding a MOZ_CRASH()) with --spectre-mitigations=off.

Filed bug 1691919 for this. Other than that, everything in comment 0 has now been addressed.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Bug 1688913 part 11 - Rename jit-test/tests/large-buffers directory to jit-test/tests/large-arraybuffers. r?lth!

This is consistent with the shell flag that was renamed to --large-arraybuffers

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aab887fbb0e9
part 11 - Rename jit-test/tests/large-buffers directory to jit-test/tests/large-arraybuffers. r=lth DONTBUILD

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/aab887fbb0e9

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f89f1881b93
part 10 - Improve ranges of MNonNegativeIntPtrToInt32 and similar instructions. r=anba,nbp

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8f89f1881b93