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

Make CuTe and PyCuTe idx2crd Behaviors The Same #1891

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leimao
Copy link
Contributor

@leimao leimao commented Oct 22, 2024

CuTe C++ has two function overloadings for idx2crd:

However, PyCuTe only has one function for idx2crd:

If default value None if used for the idx2crd(idx, shape, stride=None) in PyCuTe, the function behavior is different from the crd2idx(Coord const& coord, Shape const& shape) in CuTe C++.

Therefore, a new function idx2crd(idx, shape) was added to PyCuTe, and the default value was removed from idx2crd(idx, shape, stride=None).

@leimao
Copy link
Contributor Author

leimao commented Oct 23, 2024

@hwu36

@hwu36
Copy link
Collaborator

hwu36 commented Oct 23, 2024

@ccecka

@ccecka
Copy link

ccecka commented Oct 24, 2024

I actually recommend removing idx2crd(idx, shape, stride) completely, it is dangerous and only works when the strides are compact. Ideally, this would be removed from both python and C++.

This python implementation of idx2crd(idx, shape) also appears to be incorrect and inconsistent with the C++ version in the case that idx >= size(shape).

@manishucsd
Copy link
Contributor

manishucsd commented Oct 24, 2024

I actually recommend removing idx2crd(idx, shape, stride) completely, it is dangerous and only works when the strides are compact. Ideally, this would be removed from both python and C++.

In the case of removal of idx2crd and it also being incorrect, what would be the best and correct way to move from idx space to crd space, using manual decomposition of col-wise idx into modes or some other way?

@leimao
Copy link
Contributor Author

leimao commented Oct 24, 2024

I actually recommend removing idx2crd(idx, shape, stride) completely, it is dangerous and only works when the strides are compact. Ideally, this would be removed from both python and C++.

This python implementation of idx2crd(idx, shape) also appears to be incorrect and inconsistent with the C++ version in the case that idx >= size(shape).

I did not define idx2crd(idx, shape) for the cases that id >= size(shape). I only defined the conversion for the isomorphism in Definition 2.3 in "A note on the algebra of CuTe Layouts". If you want the definition for the extended isomorphism, i.e., the cases id >= size(shape), I can also accommodate it in the next patch.

Just let me know your decisions.

BTW, why the PyCuTe functions are not Python bindings to the CuTe C++ functions?

@ccecka
Copy link

ccecka commented Oct 24, 2024

In the case of removal of idx2crd and it also being incorrect, what would be the best and correct way to move from idx space to crd space, using manual decomposition of col-wise idx into modes or some other way?

You simply can't do it in general (without solving certain Diophantine equations...). Obviously it's not even unique for any layout with a stride-0.

I did not define idx2crd(idx, shape) for the cases that id >= size(shape)

Yes, but those cases need to be consistent as well else you run into problems in defining composition and out-of-bounds tiling.

BTW, why the PyCuTe functions are not Python bindings to the CuTe C++ functions?

It was a playful alternate implementation that got released for some reason. I still use it for prototyping or code golfing once in a while, but that's it.

@manishucsd
Copy link
Contributor

BTW, why the PyCuTe functions are not Python bindings to the CuTe C++ functions?

Having a fully functional PyCuTe support same as CuTe C++ will be extremely useful.

You simply can't do it in general (without solving certain Diophantine equations...).

So the layout operator will take me from coordinate space to index space, is there a reliable way in CuTe C++ to go in the reverse direction from index space to coordinate space? Does CuTe C++ solve Diophantine equations?

@ccecka
Copy link

ccecka commented Oct 24, 2024

No, there is no fully reliable way to go from index space to coordinate space. We now use Coordinate Tensors to track and manipulate coordinate spaces. This is how we perform predication generically among other applications. See
https://github.com/NVIDIA/cutlass/blob/main/media/docs/cute/0y_predication.md
https://github.com/NVIDIA/cutlass/blob/main/media/docs/cute/0z_tma_tensors.md

@leimao
Copy link
Contributor Author

leimao commented Oct 24, 2024

@ccecka So I guess my action item is to define idx2crd(idx, shape) for the cases that id >= size(shape) and add some unit tests, if you guys don't have a plan to add Python bindings for CuTe C++ in the near future?

Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

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

Successfully merging this pull request may close these issues.

4 participants