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

Handle intrinsics with constraints in the test tool. #1237

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

JamieCunliffe
Copy link
Contributor

Rather than using the csv export from the tracking spreadsheet we now
use the csv from the ACLE repository. This data contains any
constraints that the intrinsic has.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link

⚠️ Warning ⚠️

  • These commits modify submodules.

@JamieCunliffe
Copy link
Contributor Author

missing.txt contains a list of intrinsics that can't be tested.

Line 1-26 don't appear to exist in clang++-14 for me.
Line 26-82 don't appear to exist here.
Line 83-106 don't appear to be in clang++-12
Line 107-115 They do exist, however, they take a very long time to compile in Rust. C++ seems to handle them fine though.

I'm expecting CI to fail due to differences, patches to follow to address those.

@pthariensflame
Copy link
Contributor

Is there any reason not to use the current ACLE head as a submodule, instead of one a week old? There are a few updates to A32 support in the CSV.

@@ -0,0 +1,3 @@
[submodule "crates/intrinsic-test/acle"]
path = crates/intrinsic-test/acle
url = https://github.com/ARM-software/acle.git
Copy link
Member

Choose a reason for hiding this comment

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

For reference this submodule is currently ~10MB.

@SparrowLii
Copy link
Member

SparrowLii commented Oct 25, 2021

missing.txt contains a list of intrinsics that can't be tested.

This is good. There are some inconsistencies in the spreadsheet. I think we can use this list to track the missing instructions

@JamieCunliffe
Copy link
Contributor Author

@pthariensflame No reason for using that version of the ACLE, it was just when I added the submodule. From looking at the changes they wouldn't impact this tool as we are only testing on the aarch64-unknown-linux-gnu target so A32 isn't touched, and we don't actually use the column that the changes were in.

@Amanieu
Copy link
Member

Amanieu commented Nov 4, 2021

LGTM but there still seems to be some failing tests.

@JamieCunliffe
Copy link
Contributor Author

I just opened #1246 once that has been merged I'll rebase this and then all the tests should pass.

Rather than using the csv export from the tracking spreadsheet we now
use the csv from the ACLE repository. This data contains any
constraints that the intrinsic has.
@JamieCunliffe JamieCunliffe force-pushed the intrinsic-test-constraints branch from 680df32 to a33194d Compare November 4, 2021 14:43
@Amanieu Amanieu merged commit 0671779 into rust-lang:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants