-
Notifications
You must be signed in to change notification settings - Fork 301
Amends CI and tests for arm intrinsics to enable tests in debug mode #1949
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
base: master
Are you sure you want to change the base?
Amends CI and tests for arm intrinsics to enable tests in debug mode #1949
Conversation
This fixes build issues associated with failing LLVM const param assertions
b1633ae to
0ec44c2
Compare
0ec44c2 to
d15e102
Compare
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.
Can you give some background on why the tests did not pass in debug mode? I'd also personally like to merge the changes to the intrinsics separately from changes to the CI infrastructure
| cargo run "${INTRINSIC_TEST}" "${PROFILE}" \ | ||
| cargo run "${INTRINSIC_TEST}" --release \ | ||
| --bin intrinsic-test -- intrinsics_data/arm_intrinsics.json \ | ||
| --runner "${TEST_RUNNER}" \ | ||
| --cppcompiler "${TEST_CXX_COMPILER}" \ | ||
| --skip "${TEST_SKIP_INTRINSICS}" \ | ||
| --target "${TARGET}" | ||
| --target "${TARGET}" \ | ||
| --profile "${PROFILE}" |
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.
Combining both --release and --profile looks suspicious. What does that do?
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.
Fair point, it not obvious.
It runs the intrinsic test tool itself in release mode, and then passes the $PROFILE env-var as an argument to the tool. The tool then compiles and runs a series of rust test programs with the specified profile, comparing the outputs against the equivalent C programs.
I could add a comment explaining this?
Thanks
| let profile_dir = match profile { | ||
| "dev" => "debug", | ||
| _ => "release", | ||
| }; |
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.
I think we should explicitly match release here and just panic on anything else to make future debugging easier.
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.
That makes sense, will do.
Thanks
The offending intrinsics were passing arguments to the linked llvm intrinsics that were then failing LLVMs internal assertions. LLVM requires certain arguments to be const for some of its intrinsics, and in debug mode these arguments were not const. However, in release due to things being inlined and / or being optimised away the relevant arguments became const anyway. |
Previously, the CI has only been running the tests for the arm intrinsic in release mode.
When the CI is modified to fix this, it is discovered that the tests do not build in debug for arm targets.
This PR amends the CI, test tool, and the intrinsics themselves to address the above issues.