Skip to content

Conversation

@ayylol
Copy link
Contributor

@ayylol ayylol commented Feb 4, 2025

No description provided.

@ayylol ayylol requested a review from a team as a code owner February 4, 2025 18:57
@ayylol
Copy link
Contributor Author

ayylol commented Feb 4, 2025

Note for reviewers, a majority of asan tests were never built for non-spir triples, but from my testing they are able to compile fine for nvptx other than the two tests changed in this pr. I'm doing this to enable prebuilt e2e testing on nvidia for precommit ci #16813.

It's likely that more markup will be needed to disable the other triples for these tests, once I work on enabling prebuilt e2e testing for AMD, but I just wanted to do the smallest needed changes here.

@AllanZyne
Copy link
Contributor

Hi @ayylol, although these tests can be built on cuda or amd, we don't have plan to support them.
I think these tests are already skipped on cuda and amd now, so it should not affect your pre-ci test, right?

@ayylol
Copy link
Contributor Author

ayylol commented Feb 5, 2025

Hi @AllanZyne,

I think these tests are already skipped on cuda and amd now, so it should not affect your pre-ci test, right?

Yes, that's right they are currently skipped in the precommit ci for nvidia/amd. However the changes im trying to do is enabling nvidia for the build-only test mode, since that mode works without having access to any devices it ignores device-specific features (like cpu/gpu, backend) when taking into account the requirement statements. So instead to mark a test as not being able to compile there are the target-* features, which do work in the build-only mode as well.

although these tests can be built on cuda or amd, we don't have plan to support them.

Would it be preferable then to disable all of these for the non-spir triples? I can add the REQUIRES: target-spir to the lit.local.cfg file so that it applies to all tests in this folder.

@ayylol ayylol changed the title [SYCL][E2E] Require target-spir in two Address Sanitizer tests that fail to build for nvptx [SYCL][E2E] Require target-spir in AddressSanitizer tests Feb 5, 2025
@AllanZyne
Copy link
Contributor

Hi @AllanZyne,

I think these tests are already skipped on cuda and amd now, so it should not affect your pre-ci test, right?

Yes, that's right they are currently skipped in the precommit ci for nvidia/amd. However the changes im trying to do is enabling nvidia for the build-only test mode, since that mode works without having access to any devices it ignores device-specific features (like cpu/gpu, backend) when taking into account the requirement statements. So instead to mark a test as not being able to compile there are the target-* features, which do work in the build-only mode as well.

Ok, I see.

although these tests can be built on cuda or amd, we don't have plan to support them.

Would it be preferable then to disable all of these for the non-spir triples? I can add the REQUIRES: target-spir to the lit.local.cfg file so that it applies to all tests in this folder.

Yep, it would be better to disable all for the non-spir triples. Thanks.

@ayylol ayylol changed the title [SYCL][E2E] Require target-spir in AddressSanitizer tests [SYCL][E2E] Mark non spir build targets as unsupported in AddressSanitizer tests Feb 6, 2025
@ayylol ayylol requested a review from wenju-he February 6, 2025 16:51
@againull againull merged commit d264f5a into intel:sycl Feb 6, 2025
15 checks passed
@ayylol ayylol deleted the asan-reqs branch February 6, 2025 17:55
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.

4 participants