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

Work around compiler bugs in HIP in LinearBVH test #442

Closed
wants to merge 6 commits into from

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Dec 23, 2020

No description provided.

@aprokop aprokop added the testing Anything to do with tests and CI label Dec 23, 2020
@@ -740,6 +740,10 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(callback_with_attachment, DeviceType,
}
}

// FIXME temporary workaround bug in HIP-Clang (register spill) and SYCL
#if defined(KOKKOS_ENABLE_HIP) || defined(KOKKOS_ENABLE_SYCL)
BOOST_TEST_DECORATOR(*boost::unit_test::expected_failures(5))
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you come up with that number? I'd rather look into upgrading to ROCm 4.0

Also we need to investigate a bit more the SYCL issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the number of calls to BOOST_TEST in this test. It could be lower than that, but I see no point trying to be precise, as for me the test is dead anyway in those configurations.

test/tstLinearBVH.cpp Outdated Show resolved Hide resolved
@aprokop aprokop mentioned this pull request Dec 28, 2020
@aprokop
Copy link
Contributor Author

aprokop commented Dec 28, 2020

I think this PR can be closed as both #443 and #444 passed SYCL check, somehow. Parts of the PR modifying the testing should probably be merged, though.

@dalg24
Copy link
Contributor

dalg24 commented Dec 28, 2020

I think the cleanup of the "structured grid" unit test is a good change, but it conflicts with my upcoming reorganization of the tree unit tests

@aprokop
Copy link
Contributor Author

aprokop commented Dec 28, 2020

@dalg24 Then please go ahead and cherry pick up the changes in this PR into your upcoming PR. I'll close this if you agree.

@aprokop
Copy link
Contributor Author

aprokop commented Dec 29, 2020

I removed SYCL check, but left the HIP one. We know that HIP fails sporadically in this test due to register spills, so it's an improvement of status quo. I think it should be merged in this form.

@aprokop aprokop changed the title Work around compiler bugs in HIP and SYCL in LinearBVH test Work around compiler bugs in HIP in LinearBVH test Dec 30, 2020
@aprokop aprokop mentioned this pull request Dec 30, 2020
@aprokop
Copy link
Contributor Author

aprokop commented Dec 31, 2020

Unfortunately, CI with HIP-4.0 immediately failed after merging #447. So we need this PR. I also included the revert of the patch that removed HIP check from the test (part of #447).

@aprokop
Copy link
Contributor Author

aprokop commented Jan 4, 2021

As @dalg24 is doing the very necessary refactoring of the tests, and this PR conflicts with those changes, this will not be merged. Any interesting pieces could be cherrypicked, so not removing the branch for a while. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything to do with tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants