[CK_BUILDER] Add reflection for wmma and bwd weight instances to ck builder reflection #3592
Conversation
Snektron
changed the title
|
It's a lot easer to get comments on a draft if you can explain the what and why in your draft PR description. 😜 I think I can see what you are doing here, but generally introduce what the PR is doing and what you want feedback on. |
| builder::ElementwiseOperation output_element_op; | ||
|
|
||
| builder::GemmPadding gemm_padding; | ||
| std::optional<builder::GemmPadding> gemm_padding = std::nullopt; |
There was a problem hiding this comment.
This is really good, and you should lead your PR description with this change to ConvTraits (the "what" of the PR), as well as why we are making these optional now (the "why"). One question I have is where we should use std::optional versus using std::variant.
That's the design discussion we should focus on: how should ConvTraits be generalized for backward weights. This PR should update code comments and our relect/README.md file so that everyone understands this important generalization.
| /// @brief Helper function to report unsupported layout combinations with a clear error message. | ||
| /// @details This consteval function uses throw (not static_assert) to ensure the error is not | ||
| /// silently ignored during SFINAE. The thrown string becomes part of the compiler error message. | ||
| /// @details This consteval function is designed to fail at compile time with a descriptive |
There was a problem hiding this comment.
Let's leave the longer description. This may be obvious to you, but the fact this gets around SFINEA is an important detail. It's kind of a weird pattern, I think, and consteval throw is a new C++20 technique. I like your simplification of the comment, but I don't know how well most of our developers understand this pattern. Just a suggestion. What do you think?
| /// @brief Derives the data type from a device kernel `Instance` type. | ||
| /// Returns a `builder::DataType` enum value (e.g., FP16, BF16, FP32, BF8). | ||
| // Note: maybe move to types.hpp? | ||
| template <typename DataTypeFromInstance> |
There was a problem hiding this comment.
Thanks for deleting this redundant comment noise! I meant to shorten this comment. Also, feel free to remove the doxgen-specific markup in these comments.
|
|
||
| #pragma once | ||
|
|
||
| // Fwd instances |
There was a problem hiding this comment.
I'm trying to decide if we need this instance_to_conv_traits.hpp file. I left it in as part of the refactoring, but it may be that we can always use the specific includes. I worry some about files like this leading to longer compile times. Let me know your thoughts on this, and I'll consider this for my next refactoring and cleanup.
|
What I've read so far looks really good. My two suggestions for the PR description:
|
Draft PR to receive comments on changes made to helpers and instance traits.