-
Couldn't load subscription status.
- Fork 64
Add more static_asserts in make_block_2d_copy_CD to prevent relevant future bugs
#577
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
Conversation
…ant future bugs `make_block_2d_copy_CD` must not be called with a `void` `CopyOp`
| YMode const& y_mode) | ||
| { | ||
| static_assert(is_xe_block_2d_atom_v<CopyOp>, "Expected a block 2D atom"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need extra static assert here as you are already providing on line number 984 so if user send some old ops its will stop there itself, it will never go to 985 which will call
make_block_2d_copy_CD(CopyOp const& op, // Copy operation
TiledMMA const& mma, // TiledMMA instance
Stride<Strides...> const& gstride, // Global memory strides
XMode const& x_mode, // x, y modes
YMode const& y_mode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing!
I created this PR based on the discussion in #576 (comment).
Nevertheless, in the future, if some code-changes would result in these make_block_2d_copy_CD overloads being called from elsewhere, then these asserts should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had misread your comment. I revised the PR.
This comment was marked as resolved.
This comment was marked as resolved.
|
Had closed by mistake. |
| TiledMMA const& mma, // TiledMMA instance | ||
| Stride<Strides...> const& gstride) // Global memory strides | ||
| { | ||
| static_assert(is_xe_block_2d_atom_v<CopyOp>, "Expected a block 2D atom"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overload is also not really expected to be called directly but only by the other overload which already has the static_assert. And the equivalent overload for A/B also don't have the static_assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For A & B, CopyOp can be void, so they don't need a corresponding assert
|
I suggest to close this |
|
Not needed. |
Summary
make_block_2d_copy_CDmust not be called with avoidCopyOp, so adding correspondingstatic_asserts to prevent relevant future bugs.Doesn't solve any current issues, so maybe not high priority, but good to have.