Closed Bug 1233863 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Unspecified
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jolesen, Assigned: jolesen)

References

Details

Attachments

(7 files)

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: nobody → jolesen
Blocks: 1179514
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 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+
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+
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 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+
(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.
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+
(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.
Committed the final patch.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.