Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug configure ci #19239

Closed
wants to merge 3 commits into from
Closed

Debug configure ci #19239

wants to merge 3 commits into from

Conversation

bjacob
Copy link
Contributor

@bjacob bjacob commented Nov 21, 2024

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 Port existing ROCM ukernels from HIP to C. #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: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
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.

1 participant