ARM64: Run jit-test and jstests suites in autospider script

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jolesen, Assigned: jolesen)

Tracking

unspecified
mozilla46
ARM
Unspecified
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

3 years ago
The SM(arm64) test in Treeherder is currently only running jsapi-tests.

We should also enable the jit-test and jstests test suites, like the other SM() jobs do.

Since the ARM64 simulator is fairly slow, remove the slowest tests.
(Assignee)

Updated

3 years ago
Assignee: nobody → jolesen
Blocks: 1179514
(Assignee)

Comment 1

3 years ago
Created attachment 8700211 [details]
MozReview Request: Bug 1233863 - ARM64: Allow test to pass with --no-asmjs, and when no JIT exists. r?luke

Review commit: https://reviewboard.mozilla.org/r/28619/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28619/
Attachment #8700211 - Flags: review?(luke)
(Assignee)

Comment 2

3 years ago
Created attachment 8700212 [details]
MozReview Request: Bug 1233863 - ARM64: Don't advertise asm.js availability. r?luke

Review commit: https://reviewboard.mozilla.org/r/28621/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28621/
Attachment #8700212 - Flags: review?(luke)
(Assignee)

Comment 3

3 years ago
Created attachment 8700213 [details]
MozReview Request: Bug 1233863 - ARM64: Disable tests that require ion.enable = 1. r?jimb

Review commit: https://reviewboard.mozilla.org/r/28623/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28623/
Attachment #8700213 - Flags: review?(jimb)
(Assignee)

Comment 4

3 years ago
Created attachment 8700214 [details]
MozReview Request: Bug 1233863 - ARM64: Set up pseudo stack pointer in proglogues. r?sstangl

Review commit: https://reviewboard.mozilla.org/r/28625/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28625/
Attachment #8700214 - Flags: review?(sstangl)
(Assignee)

Comment 5

3 years ago
Created attachment 8700215 [details]
MozReview Request: Bug 1233863 - ARM64: Avoid BumpSystemStackPointer(). r?sstangl

Review commit: https://reviewboard.mozilla.org/r/28627/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28627/
Attachment #8700215 - Flags: review?(sstangl)
(Assignee)

Comment 6

3 years ago
Created attachment 8700216 [details]
MozReview Request: Bug 1233863 - ARM64: Enable jit-test and jstest suites for SM(arm64). r?sstangl

Review commit: https://reviewboard.mozilla.org/r/28629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28629/
Attachment #8700216 - Flags: review?(sstangl)

Comment 7

3 years ago
Comment on attachment 8700211 [details]
MozReview Request: Bug 1233863 - ARM64: Allow test to pass with --no-asmjs, and when no JIT exists. r?luke

https://reviewboard.mozilla.org/r/28619/#review25463
Attachment #8700211 - Flags: review?(luke) → review+

Comment 8

3 years ago
Comment on attachment 8700212 [details]
MozReview Request: Bug 1233863 - ARM64: Don't advertise asm.js availability. r?luke

https://reviewboard.mozilla.org/r/28621/#review25465
Attachment #8700212 - Flags: review?(luke) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8700262 [details] [diff] [review]
ARM64: Record the correct branch offset. r?sstangl

Don't depend on nextOffset() to get the address of the next inserted
instruction. Adding a single instruction could cause a constant pool to be
emitted first.

The b() method assembles a branch and returns its offset. Use this return value
when recording the location of a pending jump.

This fixes the MOZ_CRASH(Unrecognized jump instruction.) crashes.
Attachment #8700262 - Flags: review?(sstangl)
Comment on attachment 8700262 [details] [diff] [review]
ARM64: Record the correct branch offset. r?sstangl

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

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ -1345,5 @@
>          B(label, cond);
>      }
>      void branch(JitCode* target) {
>          syncStackPtr();
> -        addPendingJump(nextOffset(), ImmPtr(target->raw()), Relocation::JITCODE);

Just to check my understanding of the problem:

nextOffset() can't be used to get the actual offset of the next instruction, because a literal pool may be inserted.

But usually this is not a problem, because when a literal pool is inserted, it is prefixed by a guard jump instruction. So references to that next instruction instead just point to the guard jump, which forward to the actual intended instruction.

It seems like even for labels this isn't ideal, since we're binding to an indirect jump.

It also seems like this is very easy to mess up. We should definitely audit all the other uses of nextOffset(), and maybe eliminate it in favor of the pattern used in this patch.
Attachment #8700262 - Flags: review?(sstangl) → review+
Comment on attachment 8700214 [details]
MozReview Request: Bug 1233863 - ARM64: Set up pseudo stack pointer in proglogues. r?sstangl

https://reviewboard.mozilla.org/r/28625/#review25467

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp:125
(Diff revision 1)
> -    MOZ_ASSERT(!masm.GetStackPointer64().Is(sp));
> +    masm.initStackPtr();

Defining `initStackPtr()` as a NOP on other platforms would allow removing the `#ifdef JS_CODEGEN_ARM64`.
Attachment #8700214 - Flags: review?(sstangl) → review+

Updated

3 years ago
Attachment #8700215 - Flags: review?(sstangl) → review+
Comment on attachment 8700215 [details]
MozReview Request: Bug 1233863 - ARM64: Avoid BumpSystemStackPointer(). r?sstangl

https://reviewboard.mozilla.org/r/28627/#review25469

::: js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp:1573
(Diff revision 1)
> +    Mov(sp, GetStackPointer64());

nit: VIXL style uses `{ }` even for single-line `if` statements.

Comment 15

3 years ago
Comment on attachment 8700213 [details]
MozReview Request: Bug 1233863 - ARM64: Disable tests that require ion.enable = 1. r?jimb

https://reviewboard.mozilla.org/r/28623/#review25475
Attachment #8700213 - Flags: review?(jimb) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Sean Stangl [:sstangl] from comment #11)
> Comment on attachment 8700262 [details] [diff] [review]
> ARM64: Record the correct branch offset. r?sstangl
> 
> Review of attachment 8700262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm64/MacroAssembler-arm64.h
> @@ -1345,5 @@
> >          B(label, cond);
> >      }
> >      void branch(JitCode* target) {
> >          syncStackPtr();
> > -        addPendingJump(nextOffset(), ImmPtr(target->raw()), Relocation::JITCODE);
> 
> Just to check my understanding of the problem:
> 
> nextOffset() can't be used to get the actual offset of the next instruction,
> because a literal pool may be inserted.

That's right. I added a nextInstrOffset() method which works around the problem by causing a pool flush unless there's room for at least one more instruction.

In general, it seems safer to just use the return value from the assembly methods, though.

> But usually this is not a problem, because when a literal pool is inserted,
> it is prefixed by a guard jump instruction. So references to that next
> instruction instead just point to the guard jump, which forward to the
> actual intended instruction.

Yes. In this case, the guard jump would get patched instead of the intended jump. This still caused problems because the patched jump still looked like a constant pool guard to the skipPool() method. It looks of an unconditional forward branch followed by a pool header.

> It seems like even for labels this isn't ideal, since we're binding to an
> indirect jump.

Yes. Harmless, but not optimal.
 
> It also seems like this is very easy to mess up. We should definitely audit
> all the other uses of nextOffset(), and maybe eliminate it in favor of the
> pattern used in this patch.

I actually thought I had done that already, but I clearly missed a spot.
(Assignee)

Comment 18

3 years ago
Pushed all the bugfixes. Still waiting for review on the last patch which enables running the new tests.
Keywords: leave-open
Comment on attachment 8700216 [details]
MozReview Request: Bug 1233863 - ARM64: Enable jit-test and jstest suites for SM(arm64). r?sstangl

https://reviewboard.mozilla.org/r/28629/#review25561

::: js/src/devtools/automation/autospider.sh:223
(Diff revision 1)
> -    RUN_JSTESTS=false
> +    export JITTEST_EXTRA_ARGS="--jitflags=none --args=--baseline-eager -x ion/ -x asm.js/"

`--jitflags=none` is the default. Do you mean `--args="--baseline-eager --no-ion"`?

Presumably `--no-asmjs` has been provided by some other patch.
Attachment #8700216 - Flags: review?(sstangl) → review+
(Assignee)

Comment 20

3 years ago
(In reply to Sean Stangl [:sstangl] from comment #19)
> Comment on attachment 8700216 [details]
> MozReview Request: Bug 1233863 - ARM64: Enable jit-test and jstest suites
> for SM(arm64). r?sstangl
> 
> https://reviewboard.mozilla.org/r/28629/#review25561
> 
> ::: js/src/devtools/automation/autospider.sh:223
> (Diff revision 1)
> > -    RUN_JSTESTS=false
> > +    export JITTEST_EXTRA_ARGS="--jitflags=none --args=--baseline-eager -x ion/ -x asm.js/"
> 
> `--jitflags=none` is the default.

The check-jit-test target in js/src/Makefile.in prepends --jitflags=all before $JITTEST_EXTRA_ARGS. This takes quite a while to run, and many of the flag sets are redundant without Ion or asm.js enabled. Adding --jitflags=none like this overrides the first setting from the makefile.

> Do you mean `--args="--baseline-eager --no-ion"`?

We could do that, but it is not necessary. The IsIonEnabled() function always returns false for JS_CODEGEN_ARM64.
 
> Presumably `--no-asmjs` has been provided by some other patch.

Yes, that is taken care of by the "ARM64: Don't advertise asm.js availability." patch attached to this bug. Same as for --no-ion, we don't actually pass the flag but instead change the behavior of IsAsmJSCompilationAvailable().

The end result is that these tests are passing with all jitflags, whether they include --no-ion / --no-asmjs or not.
(Assignee)

Comment 22

3 years ago
Committed the final patch.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.