Closed Bug 1103450 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: dougc, Assigned: dougc)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
(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.
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).
Rebase.
Attachment #8527268 - Attachment is obsolete: true
Attachment #8527268 - Flags: review?(lhansen)
Attachment #8532808 - Flags: review?(lhansen)
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.)
Attachment #8532808 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/mozilla-central/rev/e0d4c33a5e17
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.