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

MI100 Support #24

Open
LoggerHead22 opened this issue Dec 1, 2023 · 19 comments
Open

MI100 Support #24

LoggerHead22 opened this issue Dec 1, 2023 · 19 comments

Comments

@LoggerHead22
Copy link

Hi, the documentation says that this implementation is compatible only with the MI200 and MI300 GPUs. But what about the MI100 gpu?

The code contains such conditions that formally match the MI100 with the gfx908 architecture.

bool is_gfx90x = dprops->major == 9 && dprops->minor == 0;
bool is_gfx94x = dprops->major == 9 && dprops->minor == 4;
TORCH_CHECK(is_gfx90x || is_gfx94x, "FlashAttention only supports AMD MI200 GPUs or newer.");

Will this code be compatible with MI100 in practice? If not, are there any plans to add such support? Or what are the reasons that keep you from adding support for the MI100?

@dejay-vu
Copy link

dejay-vu commented Dec 5, 2023

Hi @LoggerHead22, this code appears to be a logic fault, thanks for noting.

We haven't tested the FA on MI100 since we did most of our testing on MI250&MI300 so we are limiting the support archs. I am not sure whether it will work correctly on MI100 but you can try by adding gfx908 to the valid archs. I suppose the building process will be fine.

@LoggerHead22
Copy link
Author

Thanks for the clarification @howiejayz . Your advice really helped, the code is compiled for mi100 and runs.

However, I encountered an error during the build, which is caused by the logic of the patch.

hipified_header_filepath = HIPIFY_FINAL_RESULT[header_filepath].hipified_path
      AttributeError: 'dict' object has no attribute 'hipified_path'

This seems logical, because a dict is being created here and then we try to take its hipified_path attribute.

Replacing dict with an object of the HipifyResult class in patch helped me.

@ccbadd
Copy link

ccbadd commented Jan 11, 2024

Has this patch been merged to the main branch or do we need to apply it in order to test?

@ehartford
Copy link

I need mi100 support

@ehartford
Copy link

Hi @LoggerHead22, this code appears to be a logic fault, thanks for noting.

We haven't tested the FA on MI100 since we did most of our testing on MI250&MI300 so we are limiting the support archs. I am not sure whether it will work correctly on MI100 but you can try by adding gfx908 to the valid archs. I suppose the building process will be fine.

If you need hardware for testing mi100, I volunteer my server for this purpose. I have 8x mi100 with infinity fabric.

[email protected]

@ehartford
Copy link

Hi @sabreshao @howiejayz can you please give me a path forward?

I have a bunch of mi100s and I would like them to be hot. Without flash attention, I am blocked.

Maybe you could show me where in the code I would add it? give me some advice?

@dejay-vu
Copy link

Hi @LoggerHead22, this code appears to be a logic fault, thanks for noting.

We haven't tested the FA on MI100 since we did most of our testing on MI250&MI300 so we are limiting the support archs. I am not sure whether it will work correctly on MI100 but you can try by adding gfx908 to the valid archs. I suppose the building process will be fine.

Hi @ehartford! Currently I have no time to test FA on MI100 but could you try build and run based on this comment?

@TNT3530
Copy link

TNT3530 commented Jan 25, 2024

I was able to compile flash attention for the MI100 using the docker image. Simply adding gfx908 to the target arch array (or in my case, removing everything BUT native and gfx908) makes it run fine. (Note: this also applies to the vLLM ROCm docker image, which was my use case)

Attempts to compile outside of docker seem to fail on ROCm 6.0 due to this issue, though I was unable to downgrade back to 5.7 to test on my machine.

@luizanao
Copy link

I managed to build MI100 (gfx908) as well but the env var didn't work @TNT3530 . This is because the setup is protected against unknown architectures and gfx908 is not listed. I will open a PR for adding that since gfx908 definitely works.

@luizanao
Copy link

luizanao commented Jan 26, 2024

Here's my PR, you folks might benefit from it: #38

@ehartford
Copy link

ehartford commented Feb 26, 2024

How do I install flash attention for mi100? How is the procedure from the README.md different?

@luizanao
Copy link

luizanao commented Feb 26, 2024

@ehartford passing the card arch to the build should be enough: export GPU_ARCHS="gfx908"

@gittb
Copy link

gittb commented Jul 25, 2024

Also curious if support for Mi100 was finalized.

@ehartford
Copy link

This is awesome! Can't wait to try it!

@jamestwhedbee
Copy link

just realized Mi100 support was removed

@jamestwhedbee
Copy link

@jayz0123 was that intentional

@IMbackK
Copy link

IMbackK commented Nov 3, 2024

I can confirm that when this is patched away again to allow mi100 to build the package, the latest main builds and works fine on gfx908 at least for the dimensions i tried. So this restriction seams pretty silly, and its quite puzzling why mi100 was removed from the array again given it still works fine.

@ehartford
Copy link

Then - someone doesn't want it to work on mi100

@ehartford
Copy link

I can confirm that when this is patched away again to allow mi100 to build the package, the latest main builds and works fine on gfx908 at least for the dimensions i tried. So this restriction seams pretty silly, and its quite puzzling why mi100 was removed from the array again given it still works fine.

Could you please make a PR that enables mi100 so I can test it?

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

10 participants