Skip to content

[SYCL][CUDA] Eliminate incorrect assertions and enable cuda usm tests #2557

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

Merged
merged 4 commits into from
Sep 29, 2020

Conversation

jbrodman
Copy link
Contributor

Cuda plugin basically had divide by 0 assertions that weren't necessary.

Signed-off-by: James Brodman james.brodman@intel.com

Signed-off-by: James Brodman <james.brodman@intel.com>
@jbrodman jbrodman requested review from a team as code owners September 28, 2020 20:02
Signed-off-by: James Brodman <james.brodman@intel.com>
Signed-off-by: James Brodman <james.brodman@intel.com>
@bader
Copy link
Contributor

bader commented Sep 29, 2020

SYCL :: basic_tests/queue/queue_parallel_for_generic.cpp now works as well.

romanovvlad
romanovvlad previously approved these changes Sep 29, 2020
Signed-off-by: James Brodman <james.brodman@intel.com>
@jbrodman
Copy link
Contributor Author

Anyone know what these new XPASSes are about?

@bader
Copy link
Contributor

bader commented Sep 29, 2020

Anyone know what these new XPASSes are about?

These tests were disabled by @vmaksimo in #2505. I suppose that recent driver (#2504) update might "fix" those.
@vmaksimo, could you check if these are passing and update the expected result, please?

Related issue - #2547.

@vmaksimo
Copy link
Contributor

Anyone know what these new XPASSes are about?

These tests were disabled by @vmaksimo in #2505. I suppose that recent driver (#2504) update might "fix" those.
@vmaksimo, could you check if these are passing and update the expected result, please?

Related issue - #2547.

Looks like you are right and the issue is gone with the new GPU driver. I've created a PR with removing XFAILs to check this #2564

@bader bader merged commit da8929e into intel:sycl Sep 29, 2020
@bader
Copy link
Contributor

bader commented Oct 2, 2020

@jbrodman, FYI.
Alternative fix was proposed here: #1577.

@@ -4133,7 +4133,7 @@ pi_result cuda_piextUSMHostAlloc(void **result_ptr, pi_context context,
} catch (pi_result error) {
result = error;
}
assert(reinterpret_cast<std::uintptr_t>(*result_ptr) % alignment == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the issue when alignment == 0 - but removing it there are no checks that the allocation respects the alignment ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fwyzard, do you suggest adding something like:

assert(alignment && (reinterpret_cast<std::uintptr_t>(*result_ptr) % alignment == 0));

Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually thinking

assert(alignment == 0 || reinterpret_cast<std::uintptr_t>(*result_ptr) % alignment == 0);

since alignment == 0 means no alignment requirements IIRC ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. This seems to be an easy change: the tests are already enabled, so it should be just three new lines of code in pi_cuda.cpp.

Copy link
Contributor

Choose a reason for hiding this comment

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

#2831 recovers assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA back-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants