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]: FMHA change of drop_seed_offset to std::variant is breaking builds #1638

Closed
iratebadger opened this issue Nov 5, 2024 · 3 comments

Comments

@iratebadger
Copy link

Problem Description

The change of drop_seed_offset from a std::pair<uint64_t, uint64_t> to a std::variant<std::pair<uint64_t, uint64_t>, std::pair<const void*, const void*>> is breaking builds in a trivial manner. This should have become 2 or more constructors one with each of the types that is then passed to a std::variant version explicitly. As it stands it requires some kind o ugly code for end users.

Operating System

24.04

CPU

Ryzen 7 / NA

GPU

AMD Instinct MI100

Other

No response

ROCm Version

ROCm 6.0.0

ROCm Component

Composable Kernel

Steps to Reproduce

Pull latest xformers or flash_attention and try to build using latest CK

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

No response

Additional Information

This is a fixable issue but it requires all end users to change their code and I think there is a fix here that could result in better outcomes.

@schung-amd
Copy link
Contributor

Hi @iratebadger, slight clarification: it looks to me like prior to this change the input was an std::tuple, not std::pair, is that correct? I think a pair should be implicitly converted to the variant, but the tuple would not and I can take a look at adding back in support for that.

@poyenc
Copy link
Contributor

poyenc commented Nov 18, 2024

Hi @iratebadger could you provide an example to demonstrate how you invoke the MakeKargs() method? For I know, passing std::make_pair() argument should be fine. But constructing std::variant<> parameter directly from list initializer is not possible now.

@schung-amd
Copy link
Contributor

Backward compatibility with list initializers and tuples added by #1689 and #1681, which should resolve this issue. Feel free to comment if further related issues arise and we can reopen if necessary. Thanks for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants