1103450 - Odin atomic and locks: optimize array access when the index is known to be within bounds.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The patches in Bug 1073096 currently enforce the use of a bounds check, even when the index is know to be within bounds.

For the x64 these operations need to emit an inline bonds check when needed, rather than using the sigsegv handler to handle OOB accesses, but can still avoid the bounds check when known to be within bounds.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Optimize array access when the index is known to be within bounds.

Comment 2

[Luke Wagner [:luke]]

IIRC, the issue is that the fault handler has to know the exact offset of every atomic load/store insn and, currently, these are part of macro codegen functions so that simply taking 'masm.size()' before the codegen captures the wrong offset.  It'd be nice to have OOB tests that would have caught this in your local testing.

Comment 3

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #2)
> IIRC, the issue is that the fault handler has to know the exact offset of
> every atomic load/store insn and, currently, these are part of macro codegen
> functions so that simply taking 'masm.size()' before the codegen captures
> the wrong offset.  It'd be nice to have OOB tests that would have caught
> this in your local testing.

Perhaps I am missing something? Could you please give an example to help.

It appears that the patched paths emit code with explicit bounds checks, even on the x64. If the access is proven to be within bounds then it will not fault so the bounds check can be avoided. If the access might be OOB then the inline bounds check will be emitted so again there is no fault.

Comment 4

[Luke Wagner [:luke]]

Sorry, you're right; I was conflating the meaning of 'needsBoundsCheck' in AsmJS(Load|Store)Heap (where it is true even if not proven in-bounds on x64) with the meaning in the Atomics MIR nodes with your patch (where it is true iff proven in-bounds).

Comment 5

[Douglas Crosher [:dougc]]

Attachment: Optimize array access when the index is known to be within bounds.

Rebase.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8532808 [details] [diff] [review]
Optimize array access when the index is known to be within bounds.

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

Yes.  My mistake in the original patch was not to examine exactly how the needsBoundsCheck flag was set.

(Sorry for the delay, crazy work week.)

Comment 7

[Douglas Crosher [:dougc]]

Requesting checkin. Try build looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3fe118f5b277

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d4c33a5e17

Comment 9

[Carsten Book [:Tomcat]]

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