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 min(size_t, int) in ContractionSolution.cpp causes ambiguity #1977

Closed
yxsamliu opened this issue Jul 18, 2024 · 1 comment
Closed
Labels
change request A code change requested by external or internal stakeholders triaged The issue has been reviewed by a team member and prioritized

Comments

@yxsamliu
Copy link
Contributor

yxsamliu commented Jul 18, 2024

Problem Description

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

https://github.com/ROCm/Tensile/blame/6aa9c335a5cecf573dc129adda79e284ac898904/Tensile/Source/lib/source/ContractionSolution.cpp#L1874

Basically this line is calling function min defined in clang header with (unsigned long, 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(unsigned long, int) because that would do implicit conversion from int to unsigned long then compare. This may cause unexpected results, e.g. min(1UL, -1) will return 1UL. We want users to do explicit cast to indicate their intention. We think it is better than doing silent implicit conversion.

Therefore we would like Tensile to modify the line https://github.com/ROCm/Tensile/blame/6aa9c335a5cecf573dc129adda79e284ac898904/Tensile/Source/lib/source/ContractionSolution.cpp#L1874

to be

return min(cuCount, static_cast<size_t>(pAMDGPU->skMaxCUs));

Operating System

Ubuntu 22.04

CPU

any

GPU

AMD Instinct MI300X

ROCm Version

ROCm 6.1.0

ROCm Component

Tensile

Steps to Reproduce

No response

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

No response

Additional Information

No response

@bstefanuk bstefanuk added the change request A code change requested by external or internal stakeholders label Aug 2, 2024
@bstefanuk bstefanuk changed the title [Issue]: calling min(size_t, int) in ContractionSolution.cpp causes ambiguity [Issue]: Calling min(size_t, int) in ContractionSolution.cpp causes ambiguity Aug 2, 2024
@bstefanuk bstefanuk added the triaged The issue has been reviewed by a team member and prioritized label Aug 2, 2024
@bstefanuk
Copy link
Contributor

Updates have been applied to address this issue in #2003.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request A code change requested by external or internal stakeholders triaged The issue has been reviewed by a team member and prioritized
Projects
None yet
Development

No branches or pull requests

2 participants