NVRTC NVFP4 quantization kernels - #3301
Conversation
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
for more information, see https://pre-commit.ci
Greptile SummaryThe PR moves NVFP4 4over6 quantization kernels to runtime NVRTC compilation while retaining an optional statically compiled fallback.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NVFP4 4over6 cast request] --> B{Legacy static kernel built?}
B -- No --> C[NVRTC dispatch]
B -- Yes --> D{NVRTC enabled?}
D -- Yes --> C
D -- No --> E[Static kernel dispatch]
C --> F[Build specialization cache key]
F --> G{Kernel cached for device?}
G -- No --> H[Compile architecture-specific kernel]
G -- Yes --> I[Launch cached kernel]
H --> I
E --> J[Launch statically instantiated kernel]
I --> K[Shared quantize_4over6 body]
J --> K
Reviews (2): Last reviewed commit: "add to CMake" | Re-trigger Greptile |
|
/te-ci |
| return global_encode_scale; | ||
| } | ||
|
|
||
| #if !defined(__CUDACC_RTC__) |
There was a problem hiding this comment.
If you are not including this in the NVRTC build then how do you handle the stochastic rounding?
There was a problem hiding this comment.
This particular kernel rejects stochastic rounding
NVTE_CHECK(!quant_config->stochastic_rounding,
"NVFP4 4over6 quantization does not support stochastic rounding.");
We can do a guard like this, or move this get_rbits to a separate header which we then include in the other kernels which use it.
| @@ -0,0 +1,26 @@ | |||
| /************************************************************************* | |||
There was a problem hiding this comment.
Could we have that in some directory rather than in the top-level include directory?
| scale_stride_t, noop); | ||
| } | ||
|
|
||
| #if !defined(__CUDACC_RTC__) |
There was a problem hiding this comment.
Could we instead split this file so that the kernel is in its own header included in this file?
There was a problem hiding this comment.
Agreed. I split this out into its own header to minimize the code that we pass to rtc. This naturally resolved a lot of the other comments you had regarding the amount of RTC guards I added.
| #ifndef TRANSFORMER_ENGINE_CAST_NVFP4_RTC_DISPATCH_CUH_ | ||
| #define TRANSFORMER_ENGINE_CAST_NVFP4_RTC_DISPATCH_CUH_ | ||
|
|
||
| #if !defined(__CUDACC_RTC__) |
There was a problem hiding this comment.
Considering this entire file is excluded, why do we even try to pass it to nvrtc? Also why is it .cuh extension? It does not seem to actually need to be CUDA header, but rather regular c++ header, no?
There was a problem hiding this comment.
True, this was not actually being passed in the header list for nvrtc, so there was no reason for the .cuh
| #if FP4_TYPE_SUPPORTED | ||
| #include <cuda_fp4.h> | ||
| #endif // FP4_TYPE_SUPPORTED |
There was a problem hiding this comment.
Snippets like these should just live outside the RTC macro rather than being duplicated in 2 code paths.
There was a problem hiding this comment.
Done, with a few other examples I found of this
|
|
||
| // Keep this matrix synchronized with NVTE_SPECIFIC_ARCHS in CMakeLists.txt. | ||
| // The runtime NVRTC version is the JIT equivalent of CUDAToolkit_VERSION. | ||
| switch (sm_arch) { |
There was a problem hiding this comment.
We don't need to actually repeat the logic of the specific archs from cmake here - since we are building those kernels at runtime on the target machine, we can just always compile for the "a" variant of the specific chip that we run on (for 90+, since that's where this became a thing). The cmake compiles to "a" or "f" in order to limit the compilation targets, here we don't have this problem.
There was a problem hiding this comment.
That way we will also have less maintenance since we will not need this switch at all.
There was a problem hiding this comment.
That makes a lot of sense - I was too focused on trying to exactly replicate the compile-time rules. Now should compile always to "a"
| static constexpr float max = 0x1.FFCp15; | ||
| }; | ||
|
|
||
| // Host-only fallback for types without an explicit specialization above. |
There was a problem hiding this comment.
Please put that together with the rest of things in type_extrema.h (just guard it), as it belongs there.
| using int8_t = signed char; | ||
| using int16_t = short int; // NOLINT(*) | ||
| using int32_t = int; // NOLINT(*) | ||
| using int64_t = long long int; // NOLINT(*) | ||
| using intptr_t = long long int; // NOLINT(*) | ||
| using uintptr_t = unsigned long long int; // NOLINT(*) |
There was a problem hiding this comment.
int16_t, int32_t, and int64_t are needed because NVRTC now has to parse ptx.cuh, which uses them, while avoiding . The rest were leftovers from the mxfp8 work, I've removed them.
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
Signed-off-by: CarlosGomes98 <carlosmiguel.gomes@live.com.pt>
2748041 to
b080615
Compare
Description
Migrate NVFP4 quantization kernels to NVRTC. #3054
These kernels represent a large chunk of TE's build time for sm100 + in the cast.cu TU.
Migrating them to NVFP4 I record a 70% reduction in build time, due to the quantize_4over6 template fan-out being removed.
Fixes # (issue)
Type of change
Changes
Checklist: