Closed Bug 1476124 Opened 6 years ago Closed 6 years ago

Implement enough ARM64 Ion code to run a simple script.

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

Details

Attachments

(1 file, 2 obsolete files)

ARM64 Ion code will be landing in small patches to make it more reviewable and to reduce the rebasing effort. The downside is that a lot of this code is untestable -- instead of guessing at what the code should be, I have been inserting TODO items. When reviewing, please gloss over those, as they'll be fixed before Ion is turned on for real.

This first patch is sufficient for ARM64 Ion to compile run a basic addition loop.

There's no obvious reviewer for ARM64 stuff, so I'm just going to farm things out pseudo-randomly. The code is mostly similar to the x64 implementation, with some small changes to fit the architecture. The nitty details were mostly done years ago when I landed the pseudo-SP code, so this should be straightforward.
Attachment #8992479 - Flags: review?(ted)
Comment on attachment 8992479 [details] [diff] [review]
0001-Implement-enough-ARM64-Ion-code-to-run-a-simple-scri.patch

Apologies, wrong reviewer -- a search for "Ted Campbell" returned the wrong result.
Attachment #8992479 - Flags: review?(ted) → review?(tcampbell)
Comment on attachment 8992479 [details] [diff] [review]
0001-Implement-enough-ARM64-Ion-code-to-run-a-simple-scri.patch

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

Exciting!

::: js/src/jit/CodeGenerator.cpp
@@ +5212,1 @@
>          Register spectreRegToZero = masm.getStackPointer();

I wonder if we should have a dummy AsRegister definition for other platforms to avoid littering ifdefs.

::: js/src/jit/arm64/CodeGenerator-arm64.cpp
@@ +46,5 @@
> +        // All non-table-based bailouts will go here.
> +        masm.bind(&deoptLabel_);
> +
> +        // Store the frame size, so the handler can recover the IonScript.
> +        masm.Mov(x30, frameSize());

This needs a comment to explain why it is safe to clobber x30/lr here. (Or maybe we should have an alias similar to ScratchReg is this is a common pattern in our ARM64 codegen).

Relatedly: Should fp here have been x29? https://searchfox.org/mozilla-central/source/js/src/jit/arm64/Assembler-arm64.h#98

@@ +241,5 @@
> +    const LAllocation* lhs = ins->getOperand(0);
> +    const LAllocation* rhs = ins->getOperand(1);
> +    const LDefinition* dest = ins->getDef(0);
> +
> +    // TODO: x64 has some ins->recoversInput() check and an OOL path.

If I'm understanding recoversInput correctly, we don't recover these inputs on other platforms that use 3-register arithmetic ops. A simple |MOZ_ASSERT(!ins->recoversInput())| should be fine here.

@@ +543,5 @@
> +        bailoutIf(cond, unbox->snapshot());
> +    } else {
> +#ifdef DEBUG
> +  #if 0 // TODO: Get this debug code working.
> +        ValueOperand input = ToValue(unbox, LUnbox::Input);

I *think* the following should work:

> Register input = ToRegister(unbox->getOperand(LUnbox::Input));
Attachment #8992479 - Flags: review?(tcampbell) → review+
Addressed review comments and got the debug code running.

There were other issues in the register definitions beyond fp -- ip0 and ip1, the AArch64 temp registers, were incorrectly set to the same register.
Attachment #8993516 - Flags: review+
Keywords: checkin-needed
When trying to land this bug, got this error:

hg qpush -a
applying 0001-Implement-enough-ARM64-Ion-code-to-run-a-simple-scri.patch
applying 0001-Bug-1476124-Implement-enough-ARM64-Ion-code-to-run-a.patch
patching file js/src/jit/CodeGenerator.cpp
Hunk #1 FAILED at 5196
Hunk #2 FAILED at 5206
2 out of 2 hunks FAILED -- saving rejects to file js/src/jit/CodeGenerator.cpp.rej
patching file js/src/jit/arm64/CodeGenerator-arm64.cpp
Hunk #1 FAILED at 38
Hunk #2 FAILED at 68
Hunk #3 FAILED at 92
Hunk #4 FAILED at 150
Hunk #5 FAILED at 168
Hunk #6 FAILED at 427
Hunk #7 FAILED at 566
7 out of 7 hunks FAILED -- saving rejects to file js/src/jit/arm64/CodeGenerator-arm64.cpp.rej
patching file js/src/jit/arm64/Lowering-arm64.cpp
Hunk #1 FAILED at 42
Hunk #2 FAILED at 69
2 out of 2 hunks FAILED -- saving rejects to file js/src/jit/arm64/Lowering-arm64.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1476124-Implement-enough-ARM64-Ion-code-to-run-a.patch

::sstangl could you please take a look?
Flags: needinfo?(sstangl)
Keywords: checkin-needed
Comment on attachment 8993516 [details] [diff] [review]
0001-Bug-1476124-Implement-enough-ARM64-Ion-code-to-run-a.patch

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

::: js/src/jit/arm64/CodeGenerator-arm64.cpp
@@ +46,5 @@
> +        // All non-table-based bailouts will go here.
> +        masm.bind(&deoptLabel_);
> +
> +        // Store the frame size, so the handler can recover the IonScript.
> +        masm.Mov(x30, frameSize());

I'd still like this use of |x30| better justified/abstracted. (In your future patches is fine)
Rebased onto m-c. There were no code changes, just some line numbers were different.

The lr usage is a holdover from ARM -- needs to be audited there also.
Attachment #8992479 - Attachment is obsolete: true
Attachment #8993516 - Attachment is obsolete: true
Flags: needinfo?(sstangl)
Attachment #8994350 - Flags: review+
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65ea32c6a5be
Implement enough ARM64 Ion code to run a simple script. r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65ea32c6a5be
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: