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

[Issue]: calling function min with (int, unsigned int) causing ambiguity #343

Open
yxsamliu opened this issue Feb 29, 2024 · 3 comments
Open
Assignees

Comments

@yxsamliu
Copy link

Problem Description

when we try to fix a compiler bug about function min by llvm/llvm-project#82956 we encountered build failure in hipCUB. Then we found the line that causes the compilation failure is

https://github.com/ROCm/hipCUB/blob/develop/hipcub/include/hipcub/backend/rocprim/device/device_spmv.hpp#L142

Basically this line is calling function min defined in clang header with (int, unsigned int). Previously the compiler only defined min(int, int). As we introduce min(unsigned, unsigned), min(long, long), min(unsigned long, unsigned long). The compiler could not choose among these candidates since there is no exact match.

We do not want to define min(int, unsigned int) because that would do implicit conversion from int to unsigned int then compare. This may cause unexpected results, e.g. min(-1, 1U) will return 1U. We want users to do explicit cast to indicate their intention. We think it is better than do silent implicit conversion.

Therefore we would like hipCUB to the line https://github.com/ROCm/hipCUB/blob/develop/hipcub/include/hipcub/backend/rocprim/device/device_spmv.hpp#L142

by modifying it to be

size_t block_size = min(num_cols, static_cast<int>(DeviceSpmv::CsrMVKernel_MaxThreads));

which will keep its original behavior, or otherwise as it suits.

Thanks.

Operating System

Ubuntu 22.04

CPU

any

GPU

AMD Radeon RX 7900 XTX

ROCm Version

ROCm 6.0.0

ROCm Component

hipCUB

Steps to Reproduce

No response

(Optional for Linux users) Output of /opt/rocm/bin/rocminfo --support

No response

Additional Information

No response

@umfranzw
Copy link
Collaborator

umfranzw commented Mar 4, 2024

Hi @yxsamliu, thanks for the information. I've created a pull request with the requested change here.

@umfranzw umfranzw closed this as completed Mar 4, 2024
@yxsamliu
Copy link
Author

it seems the fix was not working. see comment https://github.com/ROCm/hipCUB/pull/344/files#r1621185464

@yxsamliu yxsamliu reopened this May 31, 2024
@yxsamliu
Copy link
Author

The fix needs to make the two arguments having the same type. Currently, only min(int, int) is defined.

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

No branches or pull requests

2 participants