[ROCm] Flydsl decode backend - #483
Conversation
|
@q10 has imported this pull request. If you are a Meta employee, you can view this in D116064789. |
|
Seems odd that this code is in mslk/attention/fmha/flydsl but the recent backward flydsl code #467 is in mslk/attention/flydsl. |
0c514b8 to
1a329d6
Compare
Move the paged-attention decode kernel package from mslk/attention/fmha/flydsl/ to mslk/attention/flydsl/ so it sits alongside the FlyDSL forward/backward kernels added in meta-pytorch#467. This removes the file-vs-package collision with fmha/flydsl.py (the meta-pytorch#467 backward integration wrapper) and aligns on one location for FlyDSL kernels, independent of the fmha integration wrappers (flydsl_decoder.py / flydsl_splitk.py) which remain under fmha/.
1a329d6 to
3d6988a
Compare
|
This makes mslk/attention/flydsl contain two independent groups of files implementing different backends, should they be split into subdirectories? I think there was a tiny bit of shared code in the previous version of this PR, but there's none now. |
@bottler I don't think that we should take into consideration shared code while making the decision on subdirectories split. If you would ask me, I lean towards more radical repo-wide restructuring, however I just want to stick with the solution that will be clear and reasonable for this repo and doesn't break anything. So, we definitely can split the algorithms into subdirectories and I think it's reasonable, however I believe it should be a follow-up PR after this one is merged. Wdyt? |
|
Let's do something about folder structure in this PR. It just seems unfriendly to have two unrelated groups of files in the same directory (unless their names were grouped I guess). Easiest change would be to make this PR put its file in a subdir called e.g. |
…reference comments
Done restructuring in ae94844 |
Summary: Pull Request resolved: meta-pytorch#500 As per request from meta-pytorch#463 (review) Pull Request resolved: meta-pytorch#483 Reviewed By: jwfromm Differential Revision: D116064789 Pulled By: q10
As per request from #463 (review)