Skip to content

Commit c2ef7d3

Browse files
timmoon10claude
andcommitted
Fix code review issues before opening PR
- Use size_t in kernel tail loop (was int64_t) - Zero-initialize Payload before memcpy (Payload{}) - Rename Payload members to kMaxBytes/kVectorSize/kMaxVectors (linter) - Consistent at::empty shape pattern: {static_cast<int64_t>(N)} - Drop intermediate swizzled_scales_bytes variable - Add comment explaining uniform-stride assumption in transform_and_load_data_ptrs_on_device - Rename sfb_buffer -> _sfb_buffer (keepalive, not directly used) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Tim Moon <tmoon@nvidia.com>
1 parent 452ce3a commit c2ef7d3

4 files changed

Lines changed: 23 additions & 22 deletions

File tree

transformer_engine/common/util/utils.cu

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,22 @@ namespace load_value_on_device {
1919
namespace {
2020

2121
union Payload {
22-
static constexpr size_t max_bytes = 2048;
23-
static constexpr size_t vector_size = 4;
24-
uint8_t bytes[max_bytes];
25-
uint32_t vectors[max_bytes / vector_size];
22+
static constexpr size_t kMaxBytes = 2048;
23+
static constexpr size_t kVectorSize = 4;
24+
static constexpr size_t kMaxVectors = kMaxBytes / kVectorSize;
25+
uint8_t bytes[kMaxBytes];
26+
uint32_t vectors[kMaxVectors];
2627
};
2728

2829
constexpr size_t block_size = 512;
29-
constexpr size_t num_blocks = DIVUP(Payload::max_bytes / Payload::vector_size, block_size);
30+
constexpr size_t num_blocks = DIVUP(Payload::kMaxBytes / Payload::kVectorSize, block_size);
3031

3132
__global__ void __launch_bounds__(block_size) kernel(Payload payload, size_t num_bytes, void *out) {
3233
const size_t tid = blockIdx.x * blockDim.x + threadIdx.x;
33-
if (Payload::vector_size * (tid + 1) <= num_bytes) {
34+
if (Payload::kVectorSize * (tid + 1) <= num_bytes) {
3435
reinterpret_cast<uint32_t *>(out)[tid] = payload.vectors[tid];
3536
} else {
36-
for (int64_t i = Payload::vector_size * tid; i < num_bytes; ++i) {
37+
for (size_t i = Payload::kVectorSize * tid; i < num_bytes; ++i) {
3738
static_cast<uint8_t *>(out)[i] = payload.bytes[i];
3839
}
3940
}
@@ -56,15 +57,15 @@ void nvte_load_value_on_device(const void *host_ptr, void *device_ptr, size_t nu
5657
// Check pointers
5758
NVTE_CHECK(host_ptr != nullptr, "Attempting to read ", num_bytes, " bytes from a null pointer.");
5859
NVTE_CHECK(device_ptr != nullptr, "Attempting to write ", num_bytes, " bytes into a null pointer.");
59-
NVTE_CHECK(reinterpret_cast<uintptr_t>(device_ptr) % Payload::vector_size == 0,
60-
"Device pointer is not aligned to ", Payload::vector_size, " bytes.");
60+
NVTE_CHECK(reinterpret_cast<uintptr_t>(device_ptr) % Payload::kVectorSize == 0,
61+
"Device pointer is not aligned to ", Payload::kVectorSize, " bytes.");
6162

6263
// Chunk data to fit in kernel arguments and launch kernels
6364
const uint8_t *src = static_cast<const uint8_t *>(host_ptr);
6465
uint8_t *dst = static_cast<uint8_t *>(device_ptr);
65-
for (size_t offset = 0; offset < num_bytes; offset += Payload::max_bytes) {
66-
const size_t chunk_size = std::min(num_bytes - offset, Payload::max_bytes);
67-
Payload payload;
66+
for (size_t offset = 0; offset < num_bytes; offset += Payload::kMaxBytes) {
67+
const size_t chunk_size = std::min(num_bytes - offset, Payload::kMaxBytes);
68+
Payload payload{};
6869
std::memcpy(payload.bytes, src + offset, chunk_size);
6970
kernel<<<num_blocks, block_size, 0, stream>>>(payload, chunk_size, dst + offset);
7071
NVTE_CHECK_CUDA(cudaGetLastError());

transformer_engine/pytorch/csrc/extensions/utils.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ at::Tensor load_data_ptrs_on_device(const std::vector<at::Tensor> &tensors,
2727
}
2828

2929
// Allocate device buffer
30-
auto ptrs_device = at::empty({tensors.size()},
30+
auto ptrs_device = at::empty({static_cast<int64_t>(tensors.size())},
3131
at::TensorOptions().dtype(at::kLong).device(device));
3232

3333
// Load pointers on device
@@ -51,7 +51,7 @@ std::tuple<at::Tensor, std::optional<at::Tensor>> transform_and_load_data_ptrs_o
5151
if (num_tensors == 0) {
5252
// No input tensors, return tensor with no elements
5353
return {
54-
at::empty(std::vector<int64_t>{0}, at::TensorOptions().dtype(at::kLong).device(device)),
54+
at::empty({int64_t{0}}, at::TensorOptions().dtype(at::kLong).device(device)),
5555
std::nullopt};
5656
}
5757

@@ -114,10 +114,10 @@ std::tuple<at::Tensor, std::optional<at::Tensor>> transform_and_load_data_ptrs_o
114114
NVTE_ERROR("Unsupported case.");
115115
}
116116

117-
// Allocate single buffer for swizzled scales
117+
// Allocate single buffer for swizzled scales.
118+
// Uses a uniform stride since all tensors share the same scale shape.
118119
const size_t swizzled_scales_stride = roundup(scale_bytes, 16); // Align to 16 bytes
119-
const int64_t swizzled_scales_bytes = swizzled_scales_stride * num_tensors;
120-
auto swizzled_scales = at::empty({swizzled_scales_bytes},
120+
auto swizzled_scales = at::empty({static_cast<int64_t>(swizzled_scales_stride * num_tensors)},
121121
at::TensorOptions().dtype(at::kByte).device(device));
122122
uint8_t *swizzled_scales_dptr = reinterpret_cast<uint8_t *>(swizzled_scales.data_ptr());
123123

@@ -167,7 +167,7 @@ std::tuple<at::Tensor, std::optional<at::Tensor>> transform_and_load_data_ptrs_o
167167
}
168168

169169
// Load pointers on device
170-
auto ptrs_device = at::empty(std::vector<int64_t>{num_tensors},
170+
auto ptrs_device = at::empty({static_cast<int64_t>(num_tensors)},
171171
at::TensorOptions().dtype(at::kLong).device(device));
172172
nvte_load_value_on_device(ptrs_host.data(), ptrs_device.data_ptr(),
173173
num_tensors * sizeof(uint64_t),

transformer_engine/pytorch/ops/fused/backward_grouped_mlp.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,7 @@ def fuser_backward(
502502
[w._columnwise_data for w in grouped_fc2_weight],
503503
device,
504504
)
505-
fc2_sfb_ptrs, fc2_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
505+
fc2_sfb_ptrs, _fc2_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
506506
"uniform_mxfp8_columnwise_swizzle",
507507
[w._columnwise_scale_inv for w in grouped_fc2_weight],
508508
device,
@@ -661,7 +661,7 @@ def fuser_backward(
661661
[w._columnwise_data for w in grouped_fc1_weight],
662662
device,
663663
)
664-
fc1_sfb_ptrs, fc1_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
664+
fc1_sfb_ptrs, _fc1_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
665665
"uniform_mxfp8_columnwise_swizzle",
666666
[w._columnwise_scale_inv for w in grouped_fc1_weight],
667667
device,

transformer_engine/pytorch/ops/fused/forward_grouped_mlp.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ def fuser_forward(
337337
[w._rowwise_data for w in grouped_fc1_weight],
338338
device,
339339
)
340-
fc1_sfb_ptrs, fc1_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
340+
fc1_sfb_ptrs, _fc1_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
341341
"uniform_mxfp8_rowwise_swizzle",
342342
[w._rowwise_scale_inv for w in grouped_fc1_weight],
343343
device,
@@ -438,7 +438,7 @@ def fuser_forward(
438438
[w._rowwise_data for w in grouped_fc2_weight],
439439
device,
440440
)
441-
fc2_sfb_ptrs, fc2_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
441+
fc2_sfb_ptrs, _fc2_sfb_buffer = tex.transform_and_load_data_ptrs_on_device(
442442
"uniform_mxfp8_rowwise_swizzle",
443443
[w._rowwise_scale_inv for w in grouped_fc2_weight],
444444
device,

0 commit comments

Comments
 (0)