Amends CI and tests for arm intrinsics to enable tests in debug mode #1949
Conversation
CrooseGit
force-pushed
the
dev/reucru01/fix-debug-tests
branch
2 times, most recently
from
0ec44c2 to
d15e102
Compare
folkertdev
left a comment
folkertdev
left a comment
There was a problem hiding this comment.
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
ci/intrinsic-test.sh
Outdated
| 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.
Combining both --release and --profile looks suspicious. What does that do?
There was a problem hiding this comment.
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.
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.
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. |
This fixes build issues associated with failing LLVM const param assertions
CrooseGit
force-pushed
the
dev/reucru01/fix-debug-tests
branch
from
d15e102 to
2dce2b9
Compare
The intrinsics test was flagging differences in aarch64_be between rust in debug and clang in O2. It was found that rust was correct in debug, but incorrect in release, and in both cases were being compared against clang in O2 which was also incorrect. The vdot intrinsics were fixed and are now correct in rust for both release and debug. However the vcmla ones could not be as the issue lies with LLVM. Both the vdot and vcmla intrinsics were added to the skiplist as clang is still incorrect for both. LLVM issue: llvm/llvm-project#166190
CrooseGit
force-pushed
the
dev/reucru01/fix-debug-tests
branch
from
2dce2b9 to
2f3cde7
Compare
|
@folkertdev |
4 participants
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.