[CPULLVM] - Prepare for LIBCXX tests enablement RISCV. - #469
Ana Pazos (apazos) merged 13 commits into
Conversation
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
29bd159 to
01ca187
Compare
|
https://github.com/qualcomm/cpullvm-toolchain/actions/runs/28660075798 Pass for. |
| ), | ||
| XFail( | ||
| name="long-double-picolibc-aarch64", | ||
| name="ong-double-picolibc-aarch64-riscv", |
There was a problem hiding this comment.
Is this a typo? (long -> ong)
There was a problem hiding this comment.
Oops yes. Thanks for the observation.
21a360d to
9025c81
Compare
|
Would you be enabling LIBCXX tests for riscv variant: riscv32im_zba_zbb_zbc_zbs_zca_zcb_zcmp_ilp32_nothreads_nopic ? If this is planned, then I would abandon my PR #465. |
Yes Pankaj, I have those variants planned for a following PR. Due to CI timeout we are brining in riscv variants in batches. |
|
CI build pass for https://github.com/qualcomm/cpullvm-toolchain/actions/runs/28667521664/job/85022776504?pr=469 the following. ninja check-cxx-riscv32gc_ilp32d |
|
Either:
And, repeating this from elsewhere:
Either just leave test_libcxx.sh unchanged and fix it up at the end, or just add the subset of variants tested in this PR and mentioned above. |
|
And, repeating from elsewhere again--please only include variants that have |
58cf1bf to
4554507
Compare
Testing all variants added in the xfails. |
4554507 to
6558ce9
Compare
| @@ -240,7 +241,7 @@ def check_r52_warning(): | |||
| "on ARMv7-A/ARMv8 with NEON. Compiler bug; exclude until fixed.", | |||
| ), | |||
| XFail( | |||
| name="long-double-picolibc-aarch64", | |||
| name="long-double-picolibc-aarch64-riscv", | |||
There was a problem hiding this comment.
I think we should not merge xfails for different targets, just create separate xfail configs.
There was a problem hiding this comment.
If the failure/testnames/descriptions are otherwise identical, I guess I have a weak preference for merging them just to not maintain this stuff twice.
But if they diverge on any of the failure cause/testnames/descriptions, I'm definitely in favor of splitting.
There was a problem hiding this comment.
Ana Pazos (@apazos) I also have a slight preference for keeping them together to avoid duplication of cause/testnames/descriptions. If they diverge as mentioned by Jonathon Penix (@jonathonpenix) it should surely be split.
| "infrastructure limitations, not test bugs.", | ||
| ), | ||
| XFail( | ||
| name="ios-xalloc-atomic-nothreads-riscv", |
There was a problem hiding this comment.
isn't there an easier way to xfail all the tests at once without listing each?
There was a problem hiding this comment.
Excluded tests should be able to use regexes in the testnames rather than just raw names.
I don't know if there is a relatively terse regex you can write that actually captures the correct set of things though.
I guess overall this doesn't seem like the right way to be handling this--if there is this much that is broken, I think we should be:
- Checking if there is something we should be configuring in our libc++ builds for these variants to disable this sort of stuff that we currently aren't (see ex: here where we configure certain features--is there something we can disable to tell libc++ not to assume we have atomic support?).
- Checking if there is something we should be adding to libc++ to handle this sort of thing
- Not testing these variants in the meantime.
I don't think XFAIL-ing however many hundreds of tests is something we want to be doing short or long term.
There was a problem hiding this comment.
I was skeptical too about doing this, I did explore if there's anything due to config that may be causing this failure. I/Claude was not able to come up with anything that was standard and acceptable solution for this. The Same issue is seen in libcxxabi enablement https://github.com/qualcomm/cpullvm-toolchain/pull/341/changes#diff-146802d63a83077720f5c2eb7d5ec6460d2304104383ee1acd4810438d0be5b2R155
I will try some more to find any option like fix. If not at all possible for the time being, I'm in favour of not testing the variants these tests fail for.
Edit, we have a pretty simple fix for it. Added that, CI is clean now.
6558ce9 to
91d8ccc
Compare
|
CI All pass https://github.com/qualcomm/cpullvm-toolchain/actions/runs/28925976231/job/85813553924?pr=469 for the following variants.
|
79a11b2 to
6b17e2d
Compare
|
CI success for https://github.com/qualcomm/cpullvm-toolchain/actions/runs/29234971715/job/86767437393?pr=469 ninja check-cxx-riscv64gc_lp64_nopic libc++ test failures with configs without atomics are disabled issue filed Make ios_base::xalloc non-atomic with LIBCXX_ENABLE_THREADS=OFF. by efriedma-quic · Pull Request #20… ; |
Jonathon Penix (jonathonpenix)
left a comment
There was a problem hiding this comment.
Please update the description to reflect what the PR is doing/enabling.
There was a problem hiding this comment.
Again #469 (comment)
Variants without 'a' can be omitted from the list in the link for now (so, the xqci one).
There was a problem hiding this comment.
The following from the list is already removed, please check again.
riscv32im_xqci_ilp32_nothreads_nopic.
There was a problem hiding this comment.
All those variants without atomic libs support, have been removed, I believe there's none here from that list that doesn't have the atomics support.
There was a problem hiding this comment.
I say "Variants without..." only to say that the list linked above does not need to be implemented verbatim given the decisions that have since been made around variants without 'a'.
If that is done, then great! Nothing needs to change on that front.
I say "Again #469 (comment)" as again I think we need to start with a subset of these variants in test_libcxx.sh to have a chance of running these tests once this is merged.
If you disagree, that's fine--but please say so/say why.
There was a problem hiding this comment.
I say "Variants without..." only to say that the list linked above does not need to be implemented verbatim given the decisions that have since been made around variants without 'a'.
If that is done, then great! Nothing needs to change on that front.
It is. Thanks.
I say "Again #469 (comment)" as again I think we need to start with a subset of these variants in test_libcxx.sh to have a chance of running these tests once this is merged.
If you disagree, that's fine--but please say so/say why.
Done too. We schedule only the following riscv variants.
riscv32gc_ilp32d
riscv32imac_ilp32
riscv32imac_zba_zbb_ilp32_nopic
riscv32imafc_zcb_zcmp_zba_zbb_ilp32f
riscv64gc_lp64_nopic
riscv64gc_zba_zbb_lp64d_nopic
riscv64imac_lp64_nopic
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
fe13a13 to
fa1b0cb
Compare
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Ping |
Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Updated, please suggest if this works or needs any change. This was deferred as the preference to run which variants was still to be decided. |
5193d63
into
qualcomm:qualcomm-software
|
/cherry-pick 5193d63 |
|
/pull-request #598 |
…enablement RISCV. (#469) (#598) Backport 5193d63 Requested by: @jonathonpenix Signed-off-by: Pawan Nirpal <pnirpal@qti.qualcomm.com> Co-authored-by: Pawan Nirpal <pnirpal@qti.qualcomm.com>
Previously, xqci variants were omitted as we saw huge numbers of libc++ test failures in variants without 'a' (see #469) and at the time we didn't have any xqci variants with 'a'. We do now (and these variants turn on C++ libs and `ENABLE_LIBCXX_TESTS`) so add one such variant in the scheduled libc++ tests--it should be clean. Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Manage fixes for RISC-V LIBCXX tests enablement.
We add xfails for the following variants, but do not enable all of them due to CI timeout limit.
We only enable the following riscv variants.