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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Sep 23, 2020
Merged

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

merged 4 commits into from
Sep 23, 2020

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

Subscribe to Label Action

cc @bnjbvr

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
Contributor

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
Contributor

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

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
Contributor

@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

@abrown abrown abrown approved these changes

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