Closed Bug 1497698 Opened 3 years ago Closed 3 years ago

ARM64: Get basic/FPQuadCmp passing tests

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

ARM64
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [arm64:m2])

Attachments

(1 file, 3 obsolete files)

Final, small patch to get basic/QuadFPCmp finally passing tests:

```
[sstangl@localhost src]$ python jit-test/jit_test.py --tbpl dbgarm64/js/src/js basic/FPQuadCmp
[5|0|0|0] 100% ======================================================>|   3.7s
PASSED ALL
```

I'll count the number of remaining failures overnight -- it's probably still very high, but this should knock out most of the simple failures related to `--ion-eager --ion-offthread-compilation=off`.

After this it's just a matter of picking a test from the current failure list and getting it to pass, again and again until it's done.
Attachment #9015714 - Flags: review?(jitbugs)
Priority: -- → P2
Attachment #9015714 - Flags: review?(jitbugs) → review?(bbouvier)
Comment on attachment 9015714 [details] [diff] [review]
0001-Get-basic-FPQuadCmp-passing-tests.-r.patch

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

A few questions before final rubberstamp, patch looks good but the comments make me doubt.

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +369,2 @@
>  void
>  PatchJump(CodeLocationJump& jump_, CodeLocationLabel label)

Could the variable named just be named `jump`?

@@ +369,4 @@
>  void
>  PatchJump(CodeLocationJump& jump_, CodeLocationLabel label)
>  {
> +    // ARM64 is JS_SMALL_BRANCH.

Can you statically assert this somehow?

Also, doesn't it mean we need to check that the jumped-to location is in range of the jump?

@@ +369,5 @@
>  void
>  PatchJump(CodeLocationJump& jump_, CodeLocationLabel label)
>  {
> +    // ARM64 is JS_SMALL_BRANCH.
> +    // The jump always refers to an address stored in a jump table.

Is that true? I see the function `jumpWithPatch` is used in an Ion IC: https://searchfox.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#2324

::: js/src/jit/arm64/CodeGenerator-arm64.cpp
@@ +830,4 @@
>          ValueOperand input = ToValue(unbox, LUnbox::Input);
>          ScratchTagScope scratch(masm, input);
>          masm.splitTagForTest(input, scratch);
> +        masm.branch32(Assembler::Condition::Equal, scratch, Imm32(tag), &ok);

Good catch.
Comment on attachment 9015714 [details] [diff] [review]
0001-Get-basic-FPQuadCmp-passing-tests.-r.patch

Clearing review flag until review questions are answered.
Attachment #9015714 - Flags: review?(bbouvier)
Whiteboard: [arm64:m2]
This patch is a partial implementation of the patch that bbouvier reviewed. I took out the sketchy stuff. I wasn't able to find a testcase that requires the more complicated logic, and I don't really want to commit code that I can't test, so for the moment I've left that as a MOZ_CRASH(). It will pop up again later in a testable environment, and then we can fix the patching logic for real.

This patch is *very* important to get landed ASAP, because Ion ICs are the sole consumer of PatchJump(). Therefore, before this patch is landed, basically every test fails in ICs, and after this patch is landed, tests fail in more useful locations.

nbp, when working locally, before this is landed, you may want to apply this on your local branch.
Attachment #9015714 - Attachment is obsolete: true
Attachment #9031297 - Flags: review?(nicolas.b.pierron)
Comment on attachment 9031297 [details] [diff] [review]
0001-Bug-1497698-Partial-Implementation-of-PatchJump-.-r-.patch

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

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +305,5 @@
> +  MOZ_ASSERT(branch->IsUncondB());
> +
> +  // The branch must be able to reach the label.
> +  ptrdiff_t relativeByteOffset = next.getOffset() - branchOffset;
> +  MOZ_ASSERT(branch->IsTargetReachable(branch + relativeByteOffset));

nit: MOZ_RELEASE_ASSERT.
Attachment #9031297 - Flags: review?(nicolas.b.pierron) → review+
Thanks! SetImmPCOffsetTarget() already has that assertion internally -- I just added a redundant MOZ_ASSERT() to get it to fail in a location that was a little easier for debugging.
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd3b9fa9eac
Partial Implementation of PatchJump(). r=nbp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bcd3b9fa9eac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Attachment #9059885 - Attachment description: added docs for the procedure of updating an expiring SSL certificate (Bug 1497698) r?sheehan → docs: added docs for the procedure of updating an expiring SSL certificate (Bug 1497698) r?sheehan

Comment on attachment 9059885 [details]
docs: added docs for the procedure of updating an expiring SSL certificate (Bug 1497698) r?sheehan

Revision D28379 was moved to bug 1496798. Setting attachment 9059885 [details] to obsolete.

Attachment #9059885 - Attachment is obsolete: true
Attachment #9059841 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.