Port existing ROCM ukernels from HIP to C. #19194
Conversation
|
Got an answer to the "to avoid the C lib or not" question right from CI:https://github.com/iree-org/iree/actions/runs/11900829477/job/33162515597?pr=19194#step:9:15741 . Looks like we are going to keep avoiding the C library after all. |
| "--rocm-path=${IREE_ROCM_PATH}" | ||
| # Target architecture/machine. | ||
| "-target" "amdgcn-amd-amdhsa" | ||
| "-march=${_ROCM_ARCH}" |
There was a problem hiding this comment.
Do we want the features as well here? (:sramecc+:xnack- is pretty common on gfx9s). I think there's a few ways to get the full string, rocm_agent_enumerator -name is the one that comes to bind.
It's probably not that big a deal, but seemed worth mentioning
There was a problem hiding this comment.
Sounds good, this is just my naive initial port using the flags from https://github.com/iree-org/iree/blob/users/benvanik/amdgpu/build_tools/cmake/iree_amdgpu_library.cmake#L49 . I hadn't heard of AMDGPU target feature sets yet, but that sounds like something we might want in the future if it's not already implied by the -march=gfx942 flags we are passing here?
There was a problem hiding this comment.
Those are optional flags I think. BTW, kind of rusty in LLVM, but since we are only using this to generate the LLVMIR which we will then link and inline to the dispatch, wouldn't we be getting/using these flags from ROCMTarget as seen here
iree/compiler/plugins/target/ROCM/ROCMTarget.cpp
Lines 91 to 94 in 540cebf
There was a problem hiding this comment.
The late post-linking passes would indeed run with those features. But enabling features also controls the semantics of the source code (e.g., if my CPU experience is any guide here, they might make certain builtins available, and make corresponding preprocessor tokens defined). So it may matter to have these features enabled at the time of the compilation of ukernels from C source. Anyway, we are already passing -march= flags, such as -march=gfx942, which, in my understanding, suffice to enable the corresponding features. For example, https://llvm.org/docs/AMDGPUUsage.html says that gfx942 implies sramecc,tgsplit,xnack,kernarg,preload. So I amn't sure when we would ever need to deal with individual features like this.
There was a problem hiding this comment.
The two ones I mention (sramecc and xnack) only come up in backend instruction selection stuff. sramecc+ means there's error-correcting SRAM (which makes certain instructions unsafe) and xnack- has to do with ... some hardware mode I forget the details of, It's debugging related, but I know that leaving off the xnack- can have, at the very least, perf implications
But yeah, if you're just going to LLVM bitcode, shouldn't be needed.
There was a problem hiding this comment.
Can you confirm that these are implied by merely passing gfx942 as the target architecture (both in ukernel compilation, and in IREE codegen) ?
|
Thanks for this super awesome work Benoit! Overall this looks great, I think we're just having some discussions now, but once the tests pass I am good with landing this and then we can re-iterate on future PRs. |
bjacob
force-pushed
the
uk
branch
2 times, most recently
from
5310d5a to
bf2a0f7
Compare
| // Local replacements for HIP headers | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| static inline int __lane_id() { |
There was a problem hiding this comment.
nit: Why are our own helpers underscored? I think the alternative would be to prefix them with iree.
There was a problem hiding this comment.
Good point: double underscore is reserved for the language and its standard library. In this instance, this is simply preserving the identifiers from HIP --- so I guess the ultimate answer to your question is that HIP is its own programming language so it's free to do double underscores, and does that for the same reason as C/C++ do, to avoid clashes with user identifiers.
Good point that now that this isn't HIP anymore, we don't have to preserve the double underscores and in fact it's not really legal for us to preserve them. So I will drop them in a follow-up (I already have a stash of 4 commits on top of this, so I prefer to address this in a follow-up).
| int64_t input_offset, int64_t *outputBuffer, | ||
| int64_t output_offset, | ||
| int64_t reductionSize) { | ||
| const int warpSize = __builtin_amdgcn_wavefrontsize(); |
There was a problem hiding this comment.
Are we fine with this being a runtime value vs. doing a preprocessor define based on the exact target?
There was a problem hiding this comment.
I think there're PRs up to constant-fold this early in the compiler, but ... yeah, we know which target we're compiling for and whether it's a wave32 or a wave64 kernel (if applicable) ... handling this ourselves isn't a bad idea
There was a problem hiding this comment.
This is an as-is port of the existing HIP code. Discussion of improving on this code by turning this runtime value into a compile-time constant, is out of the scope of this PR. My purpose here is to do the right refactorings of this ukernel infra to lay a good foundation for the ukernel that I care about --- multi_mma :-)
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
It's purely device code, so it doesn't need HIP's defining feature of generating both host and device code. It can be just C code that happens to be compiled to the AMDGPU target. The flags are taken from the `users/benvanik/amdgpu` branch, `build_tools/cmake/iree_amdgpu_library.cmake`. Tested with: ``` pytest experimental/regression_suite/tests/pregenerated/test_ukernel.py -k gfx942 ``` Notes - Completely self-contained C avoids including even C standard library headers, so that we don't run into compilation failures on some CI host based on the C headers installed on it (see iree-org#19194 (comment)). - The old code used to avoid `-O3` on RDNA3 due to numerical correctness issues. Based on what we know at this point about RDNA3, numerical correctness issues are to be expected on RDNA3 and we should not refrain from -O3, instead we should relax test tolerance on RDNA3 as needed. - Types changes: - Input buffers used to be passed as (non-`const`) `T*`, changed to `const T*`. - Size parameters were passed as `size_t`. I thought let's *not* have a size type in AMDGPU ukernels, where we are dealing with multiple address spaces with different pointer widths - let's be explicit. Then the direct mapping of `size_t` would be `uint64_t` (given this is global address space), but see next point: - Switched from unsigned to signed sizes. Generally, unsigned sizes (as in `size_t`) is a legacy choice that we don't have to be bound to here, given the self-containedness of this ukernel code. And in the context of lowering MLIR to ukernel calls, the typical MLIR values that would lower to these sizes are of MLIR `index` type, and while that is signless, it is more often treated as signed than unsigned. --------- Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
It's purely device code, so it doesn't need HIP's defining feature of generating both host and device code. It can be just C code that happens to be compiled to the AMDGPU target. The flags are taken from the `users/benvanik/amdgpu` branch, `build_tools/cmake/iree_amdgpu_library.cmake`. Tested with: ``` pytest experimental/regression_suite/tests/pregenerated/test_ukernel.py -k gfx942 ``` Notes - Completely self-contained C avoids including even C standard library headers, so that we don't run into compilation failures on some CI host based on the C headers installed on it (see iree-org#19194 (comment)). - The old code used to avoid `-O3` on RDNA3 due to numerical correctness issues. Based on what we know at this point about RDNA3, numerical correctness issues are to be expected on RDNA3 and we should not refrain from -O3, instead we should relax test tolerance on RDNA3 as needed. - Types changes: - Input buffers used to be passed as (non-`const`) `T*`, changed to `const T*`. - Size parameters were passed as `size_t`. I thought let's *not* have a size type in AMDGPU ukernels, where we are dealing with multiple address spaces with different pointer widths - let's be explicit. Then the direct mapping of `size_t` would be `uint64_t` (given this is global address space), but see next point: - Switched from unsigned to signed sizes. Generally, unsigned sizes (as in `size_t`) is a legacy choice that we don't have to be bound to here, given the self-containedness of this ukernel code. And in the context of lowering MLIR to ukernel calls, the typical MLIR values that would lower to these sizes are of MLIR `index` type, and while that is signless, it is more often treated as signed than unsigned. --------- Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com> Signed-off-by: Giacomo Serafini <179146510+giacs-epic@users.noreply.github.com>
It's purely device code, so it doesn't need HIP's defining feature of generating both host and device code. It can be just C code that happens to be compiled to the AMDGPU target.
The flags are taken from the
users/benvanik/amdgpubranch,build_tools/cmake/iree_amdgpu_library.cmake.Tested with:
Notes
-O3on RDNA3 due to numerical correctness issues. Based on what we know at this point about RDNA3, numerical correctness issues are to be expected on RDNA3 and we should not refrain from -O3, instead we should relax test tolerance on RDNA3 as needed.const)T*, changed toconst T*.size_t. I thought let's not have a size type in AMDGPU ukernels, where we are dealing with multiple address spaces with different pointer widths - let's be explicit. Then the direct mapping ofsize_twould beuint64_t(given this is global address space), but see next point:size_t) is a legacy choice that we don't have to be bound to here, given the self-containedness of this ukernel code. And in the context of lowering MLIR to ukernel calls, the typical MLIR values that would lower to these sizes are of MLIRindextype, and while that is signless, it is more often treated as signed than unsigned.