Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cranelift ARM64 and Spidermonkey integration #1519

Closed
9 of 11 tasks
bnjbvr opened this issue Apr 16, 2020 · 7 comments
Closed
9 of 11 tasks

Cranelift ARM64 and Spidermonkey integration #1519

bnjbvr opened this issue Apr 16, 2020 · 7 comments
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! cranelift Issues related to the Cranelift code generator

Comments

@bnjbvr
Copy link
Member

bnjbvr commented Apr 16, 2020

Trying to keep track of all the remaining correctness issues in the new arm64 backend, found when running Spidermonkey test cases. I'll update this list as I find and understand more issues.

cc @julian-seward1 @cfallin

@bnjbvr bnjbvr added cranelift Issues related to the Cranelift code generator cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! labels Apr 16, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@jgouly
Copy link
Contributor

jgouly commented Apr 22, 2020

I'm working on a patch for the DIV by 0 and overflow issue.

@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 22, 2020

I'll take care of float-to-int conversions edge cases. edit: done in #1578

@cfallin
Copy link
Member

cfallin commented Apr 22, 2020

I can take the trapif / ifcmp issue -- it's sort of a gnarly corner of isel. Also happy to fix rotate_left and the saturating conversions.

cfallin added a commit to cfallin/wasmtime that referenced this issue Apr 23, 2020
Previously, `fcopysign` was mysteriously failing to pass the
`float_misc` spec test. This was tracked down to bad logical-immediate
masks used to separate the sign and not-sign bits. In particular, the
masks for the and-not operations were wrong. The `invert()` function on
an `ImmLogic` immediate, it turns out, assumed every immediate would be
used by a 64-bit instruction; `ImmLogic` immediates are subtly different
for 32-bit instructions. This change tracks the instruction size (32 or
64 bits) intended for use with each such immediate, and passes it back
into `maybe_from_u64` when computing the inverted immediate.

Addresses one of the failures (`float_misc`) for bytecodealliance#1521 (test failures)
and presumably helps bytecodealliance#1519 (SpiderMonkey integration).
@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 23, 2020

I'm now looking at rotations. There are a few tricks we can steal from Spidermonkey's implementation... done => #1584

@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 23, 2020

Re trapif, investigation shows that a heap with explicit bounds checks will generate ifadd_cout to compute base+offset, then check for overflow with trapif if the carry flag is set. I'm happy to let others figure the right fix for this (this would require implementing ifadd_cout too, or lowering heap accesses with explicit bounds checks to something entirely different). Here's a test case extracted from Spidermonkey test suite.

function u0:0(i32, i64 vmctx) baldrdash_system_v {
    gv0 = vmctx
    gv1 = load.i64 notrap aligned gv0
    gv2 = load.i32 notrap aligned gv0+8
    heap0 = dynamic gv1, min 0x0001_0000, bound gv2, offset_guard 0xfff8, index_type i32

                                block0(v0: i32, v1: i64):
@0025                               v4 = load.i32 notrap aligned v1+8
@0025                               v5 = iconst.i32 0x7fff_ffe1
@0025                               v6, v7 = iadd_ifcout v0, v5
@0025                               trapif ult v7, heap_oob
@0025                               v8 = icmp ugt v6, v4
@0025                               brz v8, block3
@0025                               jump block2

                                block2:
@0025                               trap heap_oob

                                block3:
@0025                               v9 = uextend.i64 v0
@0025                               v10 = get_pinned_reg.i64 
@0025                               v2 = iadd v10, v9
@0025                               v3 = sload8.i32 v2+0x7fff_ffff
@002d                               jump block1

                                block1:
@002d                               fallthrough_return
}

cfallin added a commit to cfallin/wasmtime that referenced this issue Apr 23, 2020
Previously, `fcopysign` was mysteriously failing to pass the
`float_misc` spec test. This was tracked down to bad logical-immediate
masks used to separate the sign and not-sign bits. In particular, the
masks for the and-not operations were wrong. The `invert()` function on
an `ImmLogic` immediate, it turns out, assumed every immediate would be
used by a 64-bit instruction; `ImmLogic` immediates are subtly different
for 32-bit instructions. This change tracks the instruction size (32 or
64 bits) intended for use with each such immediate, and passes it back
into `maybe_from_u64` when computing the inverted immediate.

Addresses several of the failures (`float_misc`, `f32_bitwise`) for
 bytecodealliance#1521 (test failures) and presumably helps bytecodealliance#1519 (SpiderMonkey
integration).
cfallin added a commit to cfallin/wasmtime that referenced this issue Apr 23, 2020
Previously, `fcopysign` was mysteriously failing to pass the
`float_misc` spec test. This was tracked down to bad logical-immediate
masks used to separate the sign and not-sign bits. In particular, the
masks for the and-not operations were wrong. The `invert()` function on
an `ImmLogic` immediate, it turns out, assumed every immediate would be
used by a 64-bit instruction; `ImmLogic` immediates are subtly different
for 32-bit instructions. This change tracks the instruction size (32 or
64 bits) intended for use with each such immediate, and passes it back
into `maybe_from_u64` when computing the inverted immediate.

Addresses several of the failures (`float_misc`, `f32_bitwise`) for
 bytecodealliance#1521 (test failures) and presumably helps bytecodealliance#1519 (SpiderMonkey
integration).
cfallin added a commit to cfallin/wasmtime that referenced this issue Apr 23, 2020
Previously, `fcopysign` was mysteriously failing to pass the
`float_misc` spec test. This was tracked down to bad logical-immediate
masks used to separate the sign and not-sign bits. In particular, the
masks for the and-not operations were wrong. The `invert()` function on
an `ImmLogic` immediate, it turns out, assumed every immediate would be
used by a 64-bit instruction; `ImmLogic` immediates are subtly different
for 32-bit instructions. This change tracks the instruction size (32 or
64 bits) intended for use with each such immediate, and passes it back
into `maybe_from_u64` when computing the inverted immediate.

Addresses several of the failures (`float_misc`, `f32_bitwise`) for
 bytecodealliance#1521 (test failures) and presumably helps bytecodealliance#1519 (SpiderMonkey
integration).
@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 28, 2020

Confirmed that as of last Friday, Spidermonkey can run all the main tests, and only have the two remaining issues. I'll open up follow-up issues instead for both remaining items, because reftypes require a bunch of work in regalloc too, and heaps with explicit bounds checks is lower priority on arm64.

RESOLVED FIXED \o/

@bnjbvr bnjbvr closed this as completed Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

3 participants