-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable the wast::Cranelift::spec::simd::simd_align test for AArch64 #1802
Conversation
Subscribe to Label Actioncc @bnjbvr
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
Re: little-endian vs. big-endian -- yes, we don't have a notion of any sort of config option for aarch64be
, and a lot of other things would have to change as well, so I think the comment you left is fine.
Hmm, the CI seems to be hitting a test timeout (or just a really slow test -- our aarch64 tests run under qemu on the x86 CI hosts):
Not sure what's going on -- did tests pass for you locally? |
I usually test with |
I re-kicked the tests; likely just a spurious issue then. If still not green then I can look into the timeout settings. |
It appears there's a merge conflict now since we merged Joey's PR (#1801) -- sorry about that! |
Now the build fails for reasons that are completely unrelated to my changes:
|
Yep, I'm seeing the same CI build failure elsewhere; raised the issue on Zulip. |
@akirilov-arm we've fixed the CI issue on master, so if you rebase, the tests should pass now. Sorry about all the trouble! |
It's still not clear to me why there are failures - this is the only information that I find in the log:
In particular, the tests that my changes are supposed to enable pass. |
@cfallin I'll give it another try after the recently merged test fixes. |
I am unable to get any useful log output from the latest CI failure - it looks like a timeout? I have also tried running the tests locally on a x86 machine with QEMU 5.0.0. I have used the following command line (based on what CI is doing):
Everything completes successfully. @cfallin I am at a loss about how to proceed. It looks like everything is correct on my end and I am just blocked by CI issues. |
Sorry the CI is being so temperamental... I kicked the jobs again; let's see what happens. If still an issue, I can ping some others who know more about our CI setup. |
Job failed again with a segfault in qemu. Perhaps it's an issue with the qemu setup we have? We appear to be using version 4.2.0, according to the custom qemu build commands in our GitHub CI config. @alexcrichton , is there a way we could use a newer qemu? @akirilov-arm, is there a way you could try to replicate with 4.2.0 (to see if this is a qemu bug)? |
FWIW segfaults in qemu I've found are typically bugs in qemu (and are somewhat common, especially with things like fanciful instructions). It should be easy enough to update the copy in CI by updating this URL, and if it's working locally with 5.0.0 updating CI should do the trick! If you've got trouble doing that though I'd be happy to help |
@akirilov-arm I just rebased your commit on top of our qemu update -- let's see if this works. |
FYI QEMU 4.2.0 works for me too. However, I have one remark - I didn't notice any segmentation faults in the old log, rather |
Yeah it actually sort of looks like OOM of some form since I can't see logs from the builders. One possibility is to turn down the concurrency to just one on aarch64 tests, but otherwise it's probably ok to not enable the tests here just yet and we can figure out how to do that in CI later. |
@akirilov-arm if you want to simply re-disable the test, I'm OK with merging this now; maybe open an issue for the |
OK, I will rework the patch, so that the vector element extensions go in (we need them for some upcoming changes; for example, I have already implemented what's necessary for |
This commit also includes load and extend operations. Both are prerequisites for enabling further SIMD spec tests. Copyright (c) 2020, Arm Limited.
Thanks! |
wast::Cranelift::spec::simd::simd_address
also passes, even though there aren't any changes specifically for it.Note that this PR makes the assumption that execution happens in a little endian environment; currently the AArch64 backend doesn't seem ready to deal with big endian anyway.