hipCUB icon indicating copy to clipboard operation
hipCUB copied to clipboard

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

Open yxsamliu opened this issue 1 year ago • 3 comments

Problem Description

when we try to fix a compiler bug about function min by https://github.com/llvm/llvm-project/pull/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

yxsamliu avatar Feb 29 '24 14:02 yxsamliu

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

umfranzw avatar Mar 04 '24 20:03 umfranzw

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

yxsamliu avatar May 31 '24 15:05 yxsamliu

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

yxsamliu avatar May 31 '24 15:05 yxsamliu

This issue has been migrated to: https://github.com/ROCm/rocm-libraries/issues/93

Closing the issue in this repo. Please refer to the migrated issue for updates.

jayhawk-commits avatar May 29 '25 14:05 jayhawk-commits