-
Notifications
You must be signed in to change notification settings - Fork 102
[HIPIFY][feature] Add header injection fallback for local headers hipification #2275
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
base: amd-develop
Are you sure you want to change the base?
[HIPIFY][feature] Add header injection fallback for local headers hipification #2275
Conversation
|
Cases like CudaMath.h are a different story, as those header files are not self-contained and are not intended to be compiled as separate compilation units. The compilation unit here designed to be compiled is dxtc.cu, which is being correctly compiled by clang. FMPOV, the problem with
And not all of them necessarily need to be included during our hipification process. Such
So the question is: how should we determine whether to include this or that CUDA header file from the most likely incomplete list above? |
yes, you are correct. nvcc pre-includes the CUDA headers and headers like CudaMath.h is not designed to be a standalone compilation. But when users are trying to port using I believe we should support users even when their headers have implicit header dependencies (not only properly designed headers).
This approach avoids this problem because
whatever includes the main .cu/cpp file had before including this header - those are what the header needs here we include only header_1.h file for "sample.h". We don't need to know why it is needed just it comes before in main source file. This feature is a FALLBACK mechanism, not the primary/direct path. Here
I believe this approach works for any project (cuda-samples, etc.,) as we are only injecting original determined headers for that specific project. |
Nope. The proposed approach avoids the problems in CUDA samples, where additional header files such as The wider problem is that the necessary header file might not be provided at all. And this is the common practice in CUDA, as I tried to explain above.
Can't agree here; the approach won't work for projects that don't provide header files at all. |
You are right. I appreciate you clarifying the distinction. This approach does not solve the projects that rely entirely on nvcc's implicit pre-inclusion with no explicit system includes. I agree this is a limitation. I would like to take this feature as incremental improvement, with understanding that
I'm open to take a different approach that you'd suggest. |
|
On Windows: `error C2589: '(': illegal token on right side of '::'`:while compiling A possible reason is a Conflict: |
|
|
| #include "llvm/Support/raw_ostream.h" | ||
|
|
||
| using namespace llvm; | ||
| using namespace std; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the using namespace is declared, the corresponding namespace prefixes are surplus (below in this and other PR files).
| @@ -0,0 +1,163 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain the reason(s) for having this Std Err Capture.
I want us to think a bit broader here and come to a solution, partially based on your PR. What if we implicitly include the "short list" of already-known "hidden" CUDA headers during the hipification of each source CUDA file, as the first step?
Will we get some issues in that scenario, performance penalties, etc.? |
Motivation
https://github.com/NVIDIA/cuda-samples/blob/master/Samples/5_Domain_Specific/dxtc/dxtc.cu
The
dxtc.cucuda-samples includes multiple local headers (CudaMath.h, dds.h, permutations.h) that depend onhelper_math.hfor float3 operator definitions. Without header injection, these headers fail with standalone hipify-clang because they lack the required operator overloads.Purpose
Enhances the existing
--local-headersand--local-headers-recursiveoptions with header injection fallback mechanism. When direct hipification of a local header fails due to missing type definitions or operator overloads, the header injection method automatically injects preceding system includes from the main source file.Problem Statement
The initial local header hipification works well for self-contained headers that include all their dependencies. However, many cuda-samples have headers that depend on types and operators defined in preceding includes from the main source file. For example, CudaMath.h in cuda-samples/Samples/5_Domain_Specific/dxtc/ uses float3 operators (+=, *, -) that are defined in helper_math.h, which is included in the dxtc.cu. Few more below
bodysystemcpu_impl.h
MonteCarlo_reduction.cuh
fastWalshTransform_kernel.cuh
bicubicTexture_kernel.cuh
Solution
Implements a hybrid approach:
Existing: First attempts direct hipification, Works for self-contained headers.
Enhanced version: Header Injection, when direct method fails, it collects preceding system header files (#include <...>) from the main source and includes them.
Usage
No change to CLI flags
Non-recursive:
hipify-clang --local-headers main.cu -I include/Recursive:
hipify-clang --local-headers-recursive main.cu -I include/