Skip to content

Fix remaining failures and re-enable x64 new backend CI#2219

Merged
bnjbvr merged 4 commits intobytecodealliance:mainfrom
bnjbvr:reenable-x64
Sep 23, 2020
Merged

Fix remaining failures and re-enable x64 new backend CI#2219
bnjbvr merged 4 commits intobytecodealliance:mainfrom
bnjbvr:reenable-x64

Conversation

@bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Sep 22, 2020

See commit messages for details.

The interesting failure was actually in the implementation of nearest; the error was hidden in the old backend by the fact that the old backend uses native x86 instructions for nearest, and not the runtime intrinsic. There's a todo for this in the new backend, fwiw.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Sep 22, 2020
@github-actions
Copy link

github-actions bot commented Sep 22, 2020

Subscribe to Label Action

cc @bnjbvr

Details This issue or pull request has been labeled: "cranelift", "cranelift:area:x64"

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.

@abrown
Copy link
Member

abrown commented Sep 22, 2020

I believe gen_store_stack and gen_load_stack can be refactored to use Inst::load and Inst::store. When I created Inst::load/Inst::store, I wasn't sure about replacing the stack functions because of the signature difference but, looking at it now, it seems like all that is necessary is a let mem = SyntheticAmode::from(mem). Using Inst::load/Inst::store has the advantage of de-duplication but also uses the appropriate SSE opcodes for each vector type.

abrown
abrown reviewed Sep 22, 2020
View reviewed changes
.github/workflows/main.yml Outdated
--exclude peepmatic-runtime \
--exclude peepmatic-test \
--exclude peepmatic-souper \
--exclude lightbeam
Copy link
Member

@abrown abrown Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we use the new script?

Copy link
Member Author

@bnjbvr bnjbvr Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the new script uses cargo +nightly while we need cargo +nightly-2020-... here. I've just been a bit lazy and will implement retrieving the Cargo version with an env variable; with some luck, the other setup actions even do that for us.

abrown
abrown approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but see my comment about re-factoring gen_store_stack/gen_load_stack.

@bnjbvr
Copy link
Member Author

bnjbvr commented Sep 23, 2020

I believe gen_store_stack and gen_load_stack can be refactored to use Inst::load and Inst::store. When I created Inst::load/Inst::store, I wasn't sure about replacing the stack functions because of the signature difference but, looking at it now, it seems like all that is necessary is a let mem = SyntheticAmode::from(mem). Using Inst::load/Inst::store has the advantage of de-duplication but also uses the appropriate SSE opcodes for each vector type.

Very nice, thanks!

@bnjbvr
ci: allow passing additional parameters to run-experimental-x64-ci;
1a7d489 
This makes it possible to run a subset of the tests with e.g.
`./ci/run-experimental-x64-ci.sh -- wast::Cranelift::spec`.
@bnjbvr
machinst x64: implement loads/stores for v128 SIMD types;
8323128 
This made it possible to enable more SIMD tests from the spec test suite
too.
@bnjbvr
runtime: fix nearest for NaN inputs;
815b830 
According to wasm's spec, nearest must do the following, for NaN inputs:
- when the input is a canonical NaN, return a canonical NaN;
- when the input is a non-canonical NaN, return an arithmetic NaN.

This patch adds checks when the exponent is all ones if the input was a
NaN, and will set the significand's most significant bit in that case.
It works both for canonical inputs (which already had the bit set) and
makes other NaN inputs canonical.
@bnjbvr bnjbvr force-pushed the reenable-x64 branch 2 times, most recently from df195f6 to 7559dcf Compare September 23, 2020 13:59
@bnjbvr
Reenable the new backend x64 CI;
fd1049c 
The intermittent failure have likely been fixed by a recent round of
general fixes in the new x64 backend. This basically reverts
bytecodealliance#2100 and uses the CI
script there.
@bnjbvr bnjbvr merged commit b684384 into bytecodealliance:main Sep 23, 2020
@bnjbvr bnjbvr deleted the reenable-x64 branch September 23, 2020 14:42
@alexcrichton alexcrichton mentioned this pull request Jan 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@abrown abrown abrown approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bnjbvr @abrown