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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

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.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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/

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Attachment: 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.

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e84b83f844ce

Comment 11

[Sean Stangl [: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.

Comment 12

[Sean Stangl [:sstangl]]

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`.

Comment 13

[Sean Stangl [:sstangl]]

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 14

[Jim Blandy :jimb]

https://reviewboard.mozilla.org/r/28617/#review25473

Looks good to me.

Comment 15

[Jim Blandy :jimb]

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

Comment 16

[Jakob Stoklund Olesen [:jolesen]]

(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.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f26332167f66
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab0fdc478a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/35ea4766a62d
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c7000635b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/464b4f524094
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2200453f014

Comment 18

[Jakob Stoklund Olesen [:jolesen]]

Pushed all the bugfixes. Still waiting for review on the last patch which enables running the new tests.

Comment 19

[Sean Stangl [:sstangl]]

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.

Comment 20

[Jakob Stoklund Olesen [:jolesen]]

(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.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33a31a8434e0

Comment 22

[Jakob Stoklund Olesen [:jolesen]]

Committed the final patch.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f26332167f66
https://hg.mozilla.org/mozilla-central/rev/dab0fdc478a8
https://hg.mozilla.org/mozilla-central/rev/35ea4766a62d
https://hg.mozilla.org/mozilla-central/rev/89c7000635b9
https://hg.mozilla.org/mozilla-central/rev/464b4f524094
https://hg.mozilla.org/mozilla-central/rev/b2200453f014
https://hg.mozilla.org/mozilla-central/rev/33a31a8434e0