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

add mappings #1403

Draft
wants to merge 1 commit into
base: rocm6.2_internal_testing
Choose a base branch
from
Draft

add mappings #1403

wants to merge 1 commit into from

Conversation

SeanSong-amd
Copy link

add std and cub::BLOCK_LOAD_WARP_TRANSPOSE

Jira ID: SWDEV-458189
Fixes #ISSUE_NUMBER

add std and cub::BLOCK_LOAD_WARP_TRANSPOSE

Signed-off-by: Song <xiaosong@amd.com>
("std::fabs", ("std::fabs")),
("std::fmod", ("std::fmod")),
("std::remainder", ("std::remainder")),
("std::frexp", ("std::frexp")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jeffdaily I think we specifically chose to map the std:: functions to anonymous namespace due to some device libs issue? Do you recall?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For these math operations we do not want to use the std implementations of them but instead the implementations provided by HIP. That's what I recall.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we facing problems by not mapping them to std:: namespace?

Copy link
Author

Choose a reason for hiding this comment

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

@jeffdaily Have tested on the rocm6.0 with nightly pytorch build and rocm6.2 provided by @jithunnair-amd, the hip can't handle it properly. got some error like this "/root/AISS-Data-Science/mamba/causal-conv1d/csrc/hip/causal_conv1d_fwd.hip:36:11: error: no matching function for call to 'max'
: ::max({sizeof(typename BlockLoadT::TempStorage), sizeof(typename BlockStoreT::TempStorage)});"

Copy link
Author

Choose a reason for hiding this comment

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

It can be reproduced by this ticket https://ontrack-internal.amd.com/browse/SWDEV-458189

Copy link

@amoskvic amoskvic May 2, 2024

Choose a reason for hiding this comment

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

I was working on a similar issue recently. It appears that these std mappings are handled differently depending on whether we are talking about device vs host functions. I am not sure how it will behave in cases when a function is defined as both, as it is in https://github.com/state-spaces/mamba/blob/main/csrc/selective_scan/uninitialized_copy.cuh, for example.

It definitely fails to map properly in its present state, but I am not sure that simply adding the mapping won't break something else.

Maybe the parser gets confused for such functions?

Copy link

@amoskvic amoskvic May 2, 2024

Choose a reason for hiding this comment

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

TLDR: I suspect it might be due to a function being defined as both host and device, while these mappings are supposed to be replaced in device function only. Maybe the hipifyer does not handle the "both" case properly?

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

Successfully merging this pull request may close these issues.

None yet

4 participants