[SYCL][HIP] Fix max constant memory device query #5168
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes the `Basic/info.cpp` test for HIP AMD. The issue here is that AMD GPU report a very large constant memory (confirmed using `clinfo` as well). But the HIP entry point to query that information takes a `int*`. So the value will show up as negative, even though it's the correct value. So remove the assertion on if the value is positive and cast it back to `unsigned` before returning it.
npmiller
added a commit
to npmiller/llvm-test-suite
that referenced
this pull request
Dec 17, 2021
This will be fixed in intel/llvm#5168
sycl/plugins/hip/pi_hip.cpp
Outdated
| return getInfo(param_value_size, param_value, param_value_size_ret, | ||
| pi_uint64(constant_memory)); | ||
| pi_uint64(static_cast<unsigned int>(constant_memory))); |
There was a problem hiding this comment.
can't you just make the constant_memory be unsigned int?
There was a problem hiding this comment.
The problem is hipDeviceGetAttribute takes a int *, so we do need a cast, either here, or I can make constant_memory a unsigned int and cast the pointer when calling hipDeviceGetAttribute.
There was a problem hiding this comment.
make constant_memory a unsigned int and cast the pointer when calling hipDeviceGetAttribute
Let's do so. Not sure the cast would be really needed.
There was a problem hiding this comment.
Updated as suggested, however a cast is still needed, without it complains with the following:
llvm/sycl/plugins/hip/pi_hip.cpp:1320:31: error: invalid conversion from ‘unsigned int*’ to ‘int*’ [-fpermissive]
hipDeviceGetAttribute(&constant_memory,
^~~~~~~~~~~~~~~~
smaslov-intel
approved these changes
Jan 4, 2022
bader
pushed a commit
to intel/llvm-test-suite
that referenced
this pull request
Jan 11, 2022
This will be fixed in intel/llvm#5168
aelovikov-intel
pushed a commit
to aelovikov-intel/llvm
that referenced
this pull request
Mar 27, 2023
This will be fixed in intel#5168
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the
Basic/info.cpptest for HIP AMD.The issue here is that AMD GPU report a very large constant memory
(confirmed using
clinfoas well). But the HIP entry point to querythat information takes a
int*. So the value will show up as negative,even though it's the correct value.
So remove the assertion on if the value is positive and cast it back to
unsignedbefore returning it.