Skip to content

Replaced cublasLt calls with rocblas gemm + 6 manually crafted epilogues#3

Merged
wenchenvincent merged 7 commits into
2897-nvte-port-prior-fp8from
2897-nvte-port-gemm
Jan 9, 2023
Merged

Replaced cublasLt calls with rocblas gemm + 6 manually crafted epilogues#3
wenchenvincent merged 7 commits into
2897-nvte-port-prior-fp8from
2897-nvte-port-gemm

Conversation

@wenchenvincent

Copy link
Copy Markdown
Collaborator

No description provided.

@HaiShaw HaiShaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have some concern regarding the order of ops in backward pass, in case both bias and gelu are invoked. Minor but careful concerns are over bias dimensional sizes.
Very good in general!

Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu Outdated
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu Outdated
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu Outdated
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu
Comment thread transformer_engine/common/gemm/cublaslt_gemm.cu Outdated
wenchenvincent and others added 4 commits November 22, 2022 07:23
Fixed an issue with bias gradient not initializing.
The VectorizedUnaryKernelLaunch seems problematic in bias epilogue.
Replaced with my own identity kernel.
As a result `python main.py --use-te --use-amp` retains loss &
accuracy as `python main.py --use-te` in same scenario on CUDA
Fixed implementation of CUBLASLT_EPILOGUE_DGELU_BGRAD.
Added env var NVTE_LOG_GEMM_CONFIG to control logging of GEMM configs.
int row_idx = idx % THREADS_PER_COL;
float thread_data;
if (row_idx < m)
thread_data = (float)in[row_idx * n + col_idx];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes the row major storage, which is different than the default of remaining libraries.
I think if we change it to thread_data = (float)in[row_idx + m * col_idx] then it is easier to understand and we can unbend the confusing comment and code like belows:

   // The bias vector length is m. So it will be reduced along axis 0 in row major
   ... ...
  NVTE_CHECK_CUDA( hipMalloc(&bias_tmp, sizeof(float)*m) );   // L398
  ... ...
  NVTE_CHECK_CUDA( hipMalloc(&bias_tmp, sizeof(float)*n) );   // L450

It is confusing is because bias vector should always be length of n (from a same context of a m*n matrix), if we treat the storage formation consistently cross APIs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, we may want to unstrict us from using (float) only, onwards.
This BlackReduce seems to be templated. But it is fine for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the data layout is indeed row major as it is C/C++ convention. Only rocBlas assumes column major layout. Outside rocBlas, it is always row major.

Notice that the perception that the bias vector should always be length of n assumes that n is the number of output features. However, when we call GEMM, m, k, n have been changed and it is not always the case that n is the number of the output features.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that in many kernels of NVTE, the computation is done in fp32, while the input/output could be of lower precision. Especially for reduction operations, fp32 seems to be preferred.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the row or column major, we are not dealing with native HOST C/C++ arrays, so what C/C++ compiler uses for that is irrelevant. As far as I know, beyond rocBLAS, CK, cuBLAS, BLAS, most math libraries took column majors as default.
I'm not pointing logic errors, but mostly for code clarity here. As it stands, I see inconsistent hipMalloc - hard to follow or looks unclear to me. I'm proposing a consistent view only, hoping to make logic more clear though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, when we call GEMM, m, k, n have been changed and it is not always the case that n is the number of the output features.
It would be helpful with some comment s to tell when m & n may have changed and how.
It seems hard to follow inconsistent code like below:

  NVTE_CHECK_CUDA( hipMalloc(&bias_tmp, sizeof(float)*m) );   // L398
  ... ...
  NVTE_CHECK_CUDA( hipMalloc(&bias_tmp, sizeof(float)*n) );   // L450

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if most math libraries take column major as default. rocBlas assumes the data is column major. But in frameworks, the data is arranged in row major.

Also, the main reason why the bias vector length is sometimes m and sometimes n, is that the assumption that n is the number of output features no longer holds true in some GEMM configs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the explanation regarding why in CUBLASLT_EPILOGUE_BIAS, the bias length is GEMM_m while in CUBLASLT_EPILOGUE_BGRADB, the bias length is GEMM_n.

For a linear layer in PyTorch, we have the following equation for forward pass:

Y = X * W^T + b. X is of shape (m, k), W is of shape (n, k) and Y is of shape (m,n).

m - batch size
k - input feature dimension
n - output feature dimension.

Notice that m, k, n here are not necessarily the m, k, n used in GEMM calls. So we will denote the m, k, n used in GEMM as GEMM_m, GEMM_k, GEMM_n later.

For the forward pass, we want to compute Y = X * W^T + b, and the data in X and W are row major. But cuBlas/cuBlasLt/rocBlas assumes the data to be column major. To avoid the hassle of converting the data layout from row major to column major, what we can do is to call GEMM(W, X) with transa=T.

Here W is of shape (n,k) in row major and of shape (k, n) in column major.
X is of shape (m, k) in row major and of shape (k, m) in column major.
The result is of shape (m, n) in row major and of shape (n, m) in column major.

In the GEMM call for the forward pass, matrix A is W (with transa=T), matrix B is X, so

GEMM_m = n
GEMM_k = k
GEMM_n = m

We can see that the bias vector length n is actually GEMM_m. That's why in CUBLASLT_EPILOGUE_BIAS, we say the bias vector length is "m". GEMM call itself is not aware of the batch size, input feature dimension, or output feature dimension in the NN layer. All it knows is GEMM_m, GEMM_k, and GEMM_n.

For the backward pass, we have:

dX = dY * W
dW = dY^T * X
db = sum(dY, axis=0)

For CUBLASLT_EPILOGUE_BIAS, they compute db along with dW.

Again, notice that only when we use cuBlas/cuBlasLt/rocBlas will data be treated as column-major layout. In other contexts, they are row major. So to compute dW without actually converting the input matrices from row-major to column-major layout, we will call rocblas_gemm_ex(X, dY), with transb=T.

Here X is of shape (m, k) in row major and of shape (k, m) in column major.
dY is of shape (m, n) in row major and of shape (n, m) in column major.
The result is of shape (k, n) in column major and of shape (n, k) in row major, which is exactly dW

So in this gemm call, the matrix B is dY, and transb=T, and the matrix D is dW. db is computed by reducing dY.
We can denote the m,n,k in the GEMM for computing dW as gemm_m, gemm_n, and gemm_k. We have the following:

gemm_m = k
gemm_k = m
gemm_n = n

So here the bias vector length n is gemm_n. That's why in CUBLASLT_EPILOGUE_BIAS, the bias length is "n".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explain is perfectly correct!
I think regarding the code itself, we can add a comment to each case (within our cublas_gemm wrapper) with something like:

// gemm_ex(A=W, B=X), determined by user code in module.py
// gemm_ex(A=X, B=dy), determined by user code in module.py

to make it easier to read.
We may also define (to each different case)

math_m = m 
math_k = k
math_n = n

or

math_m = n
math_k = k
math_n = m

or 

math_m = k
math_k = m
math_n = n

assume m/k/n the gemm_m/gemm_k/gemm_n within the wrapper function.

This way (together with previous comment added) we will have an uniform and easier comprehensive code at this layer, for something like:

NVTE_CHECK_CUDA( hipMalloc(&bias_tmp, sizeof(float)*math_n) );

…ilogues to the dimensions at the NN layer level.

@HaiShaw HaiShaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

TRANSFORMER_ENGINE_TYPE_SWITCH_ROCM_SIM(output_dtype, OType,
detail::identity_kernelLauncher<float, OType>(reinterpret_cast<const float*>(bias_tmp),
reinterpret_cast<OType*>(bias_ptr),
n,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume previous 'n' should be 'm'. No question to the modified

@wenchenvincent
wenchenvincent merged commit cf5f670 into 2897-nvte-port-prior-fp8 Jan 9, 2023
@wenchenvincent
wenchenvincent deleted the 2897-nvte-port-gemm branch January 9, 2023 22:23
wenchenvincent pushed a commit that referenced this pull request Sep 17, 2024
Signed-off-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>

Signed-off-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
wenchenvincent added a commit that referenced this pull request Jul 13, 2026
Applied review comments 1, 2, 3, 4, 6, 7, 8 from PR #667 in one pass.
Comments 5 and 9 will be answered on-thread (see below).

Applied
=======

#7 cpp_extensions/gemm.py: gate NVTE_USE_GEMM_TRITON behind
   IS_HIP_EXTENSION. Our Triton kernels use gfx942/gfx950-specific MFMA
   instructions and autotune configs; refuse to enable on non-HIP
   builds.

#1 + #4 + #8: deduplicate dtype utilities against
   triton_kernels/common.py, which is the authoritative source shared
   across all Triton kernel backends here.

   - gemm_common.py: drop our copies of _get_fp8_dtypes(),
     torch_to_te_dtype(), te_to_torch_dtype(). Keep is_fp8_dtype()
     (operates on tex.DType; common.py's is_fp8_torch_dtype operates on
     torch dtypes -- different signatures), reinterpret_as_fp8_tensor,
     getGemmOutputShape, product.
   - gemm_wrapper.py: import torch_dtype_to_te_dtype /
     te_dtype_to_torch_dtype from ..common; use those instead of the
     removed local names.
   - __init__.py: drop the redundant re-exports (torch_to_te_dtype,
     te_to_torch_dtype, _get_fp8_dtypes). Note in the file directing
     callers to triton_kernels.common.
   - test_gemm_kernel.py: import get_torch_e4m3_type,
     get_torch_e5m2_type, torch_dtype_to_te_dtype from
     triton_kernels.common; define a local _get_fp8_dtypes() shim over
     them to keep the test's parametrization tables unchanged.
   - test_gemm_mxfp8.py: previously used _get_e4m3_dtype() -- dropped
     along with the test that used it (see below).

#2 + #3: add module docstrings to test_gemm.py, test_gemm_kernel.py,
   test_gemm_mxfp8.py making the scope of each file explicit and
   pointing at the sibling files. Prompted by wangye805's confusion
   between test_gemm.py and test_gemm_kernel.py, and by the question of
   whether MXFP8 is only tested in test_gemm_mxfp8.py.

#6 test_numerics.py: rewrite the comment above and the reason= text
   for _skip_grouped_under_gemm_triton to make it explicit that the
   skip is a *backend mismatch* between the two sides of the
   comparison, not Triton non-determinism.
     - sequential side -> our Triton (via NVTE_USE_GEMM_TRITON=1)
     - grouped side    -> hipBLASLt / CUTLASS / AITER-Triton grouped
                          (controlled by the SEPARATE
                          NVTE_USE_GROUPED_GEMM_TRITON env var)
   With just NVTE_USE_GEMM_TRITON=1 set, the two sides diverge in fp32
   rounding because they run different kernels. Not our Triton being
   non-deterministic.

Also removed test_mxfp8_kernel_with_simulated_data (an early kernel-
bring-up smoke test that only asserted non-zero output; MXFP8
correctness is fully covered by test_gemm.py::test_triton_vs_*_mxfp8
with real MXFP8Tensor and both PyTorch/C++ references).

To be answered on-thread
========================

#5 (conftest.py hook vs explicit skip lists) -- keep the hook: it fires
   only on three specific ValueError substrings from our own gate code,
   is self-retiring when the gates are relaxed, and explicit skips
   would require ~1300 marks. Detail in the PR reply.

#9 (Float8TensorWrapper / MXFP8TensorWrapper vs the TE tensor types) --
   defer; needs a side-by-side to see how much of the wrapper API is
   genuinely required for the Triton-kernel call surface (fields
   accessed, dimension reordering, missing-transpose fallbacks) vs.
   what could route through the TE tensor types directly.

Verified on gfx950 / PyTorch 2.10 -- all three direct suites clean:
  test_gemm.py         212 pass /  72 skip
  test_gemm_kernel.py  450 pass / 606 skip
  test_gemm_mxfp8.py     2 pass          (was 3; smoke test removed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants