Skip to content

[HIPIFY][feature] Implicit hipification of local header files - Part 1#2402

Open
ranapratap55 wants to merge 7 commits intoROCm:amd-developfrom
ranapratap55:users/ranapratap55/implict-local-headers
Open

[HIPIFY][feature] Implicit hipification of local header files - Part 1#2402
ranapratap55 wants to merge 7 commits intoROCm:amd-developfrom
ranapratap55:users/ranapratap55/implict-local-headers

Conversation

@ranapratap55
Copy link
Copy Markdown
Collaborator

@ranapratap55 ranapratap55 commented Feb 23, 2026

Problem Statement

hipify-clang currently only transforms the main source file passed as input. Local (quoted) headers included in the main file remain same as CUDA code. These headers often cannot be hipified individually because they are not self-contained headers which requires types, operators, and definitions from preceding includes in the parent source.

Example: dxtc.cu includes CudaMath.h, which uses float3 operators (+=, *, -) defined in helper_math.h. Hipifying CudaMath.h alone fails because these operators are unavailable. Similar cases:

Purpose

When hipify-clang sample.cu is run, the tool now implicitly detects and hipifies all local headers. No additional flags required.

#2402 (comment)

  1. Implement the ability to implicitly hipify all header files that are included in the main file.

This redesign is based on feedback from #2275 and earlier iterations of this PR(#2402):

  • Implicit by default: no --local-headers / --local-headers-recursive flags
  • Single opt-out: --skip-local-headers when header processing is not desired

With #2349, we addressed nvcc's implicit cuda_runtime.h. Now, with this PR(#2402) we are implicitly transforming project-level header dependencies.

Solution

Local quoted headers hipified automatically, no flags needed.
hipify-clang sample.cu -I ../../../Common/

Skip header processing if not desired
hipify-clang --skip-local-headers sample.cu -I ../../../Common/

Supersedes #2275

Summary of changes in this PR

Local header hipification is now implicit by default. hipify-clang sample.cu automatically detects and hipifies all local headers recursively, no flags needed. Added --skip-local-headers as the single opt-out mechanism.

ToDo work (follow-up PRs):

  • Warning for non-self-contained headers: Warn (besides compiler’s error messaging) the user about imposibility to hipify a header file, which is not self-contained.
  • Folder-level hipification: Hipify everything in the provided folder.

@emankov
Copy link
Copy Markdown
Collaborator

emankov commented Feb 24, 2026

Can we say that the main purpose of this PR is to allow hipifying header files individually as main translation units?

@ranapratap55
Copy link
Copy Markdown
Collaborator Author

Can we say that the main purpose of this PR is to allow hipifying header files individually as main translation units?

Yes, correct. The main purpose is to allow hipifying local header files as main translation units. We are doing it by injecting their preceding includes from the parent source file using Clang's -include ArgumentsAdjusters which is to provide the necessary compilation context.

This feature is enabled via --local-headers / --local-headers-recursive and only applies to local (quoted) headers referenced in the main source file.

@emankov
Copy link
Copy Markdown
Collaborator

emankov commented Feb 25, 2026

the parent source

How do we guarantee finding the parent source(s) for a particular header file so we can hipify it as the main translation unit?

@ranapratap55
Copy link
Copy Markdown
Collaborator Author

the parent source

How do we guarantee finding the parent source(s) for a particular header file so we can hipify it as the main translation unit?

We don't need to discover/guess the parent source file. The user provides it explicitly while hipifying. The main source file passed to hipify-clang is always the parent. --local-headers simply extends hipification to that source file's local headers, using the source file itself as the context for preceding includes.

For example: hipify-clang --local-headers sample.cu: here sample.cu is known parent for its local headers (header1.h, header2.h, etc.,).

@emankov
Copy link
Copy Markdown
Collaborator

emankov commented Feb 26, 2026

hipify-clang --local-headers sample.cu

That is what worries me most.

  1. The user should know or learn about the main source file, or, in general, the source files that include that particular header file.
  2. sample.cu is not a header file. Here we have misleading information for the end users.
  3. We need to explain somewhere, somehow, this tricky feature (actually a workaround).
  4. Finally, it makes hipify-clang even more complicated in use, which we definitely do not want, as users already consider hipify-clang complicated enough to prefer hipify-perl usage instead. And we want the opposite. We want users to prefer hipify-clang. It means that we need to simplify it AMAP and do more work under the hood implicitly.

Technically, the main problem is that we are trying to treat a file, a header file, which is not self-contained, as a source file for hipification (actually, compilation).

I want us to think about other approaches here.
For instance:

  1. Warn (besides compiler’s error messaging) the user about imposibility to hipify a header file, which is not self-contained.
  2. Implement the ability to implicitly hipify all header files that are included in the main file.
  3. Hipify everything in the provided folder.

@ranapratap55 ranapratap55 force-pushed the users/ranapratap55/implict-local-headers branch from 5e6249f to 353ebef Compare March 4, 2026 06:20
@ranapratap55 ranapratap55 changed the title [HIPIFY][feature] Local header hipification with preceding include injection via ArgumentsAdjusters [HIPIFY][feature] Implicit hipification of local header files Mar 4, 2026
@ranapratap55
Copy link
Copy Markdown
Collaborator Author

hipify-clang --local-headers sample.cu

That is what worries me most.

  1. The user should know or learn about the main source file, or, in general, the source files that include that particular header file.
  2. sample.cu is not a header file. Here we have misleading information for the end users.
  3. We need to explain somewhere, somehow, this tricky feature (actually a workaround).
  4. Finally, it makes hipify-clang even more complicated in use, which we definitely do not want, as users already consider hipify-clang complicated enough to prefer hipify-perl usage instead. And we want the opposite. We want users to prefer hipify-clang. It means that we need to simplify it AMAP and do more work under the hood implicitly.

Technically, the main problem is that we are trying to treat a file, a header file, which is not self-contained, as a source file for hipification (actually, compilation).

I want us to think about other approaches here. For instance:

  1. Warn (besides compiler’s error messaging) the user about imposibility to hipify a header file, which is not self-contained.
  2. Implement the ability to implicitly hipify all header files that are included in the main file.
  3. Hipify everything in the provided folder.

As per our discussions/suggestions, I've reworked this PR to implement suggestion 2.
Local header hipification is now implicit by default. hipify-clang sample.cu automatically detects and hipifies all local headers recursively, no flags needed. Added --skip-local-headers as the single opt-out mechanism.

I will raise 2 separate PRs as a follow up for:

  1. Warn the user for non-self-contained headers
  2. Folder-level hipification

Updated PR title and description to reflect the changes.

@ranapratap55 ranapratap55 added the partial Partial fix or implementation label Mar 20, 2026
@ranapratap55 ranapratap55 changed the title [HIPIFY][feature] Implicit hipification of local header files [HIPIFY][feature] Implicit hipification of local header files - Part 1 Mar 20, 2026
@ranapratap55 ranapratap55 force-pushed the users/ranapratap55/implict-local-headers branch from 353ebef to d10566f Compare March 24, 2026 07:43
Comment thread src/LocalHeader.cpp Outdated
Comment thread src/LocalHeader.cpp Outdated
Comment thread src/ArgParse.cpp
cl::opt<bool> OptLocalHeadersRecursive("local-headers-recursive",
cl::desc("Enable hipification of quoted local headers recursively"),
cl::opt<bool> SkipLocalHeaders("skip-local-headers",
cl::desc("Skip implicit hipification of local (quoted) headers included in the main source file"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What if the "local" headers are included with <>?

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 believe this is out of scope. Angled-bracket includes files are resolved using -I search paths. They represent system headers or external library dependencies.

But I do think there is edge case like, if a project uses #include <mylib/utils.h> via -I project/src flag, that would require resolving each angled-bracket against every -I path. That is a meaningful work and requires a separate PR of its own.

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.

#2505: Angle-bracket local header hipification (#include <>)

Comment thread src/LocalHeader.cpp Outdated
Comment thread src/LocalHeader.cpp Outdated
Comment on lines +95 to +128
bool collectPrecedingIncludes(const std::string &mainSourceAbspath,
const std::string &targetHeaderAbspath,
std::vector<std::string> &outIncludes) {
std::string mainSourceContent;
if (!readFile(mainSourceAbspath, mainSourceContent)) {
errs() << sHipify << sError << "Cannot read source files: "
<< mainSourceAbspath << "\n";
return false;
}

std::string targetFileName = std::string(sys::path::filename(targetHeaderAbspath));
std::istringstream iss(mainSourceContent);
std::string line;
std::smatch m;

while (std::getline(iss, line)) {
if (std::regex_match(line, m, LocalIncludeRe)) {
std::string quotedName = m[1].str();
std::string quotedFileName = std::string(sys::path::filename(quotedName));
if (quotedFileName == targetFileName)
break;
std::string absPath;
if (resolveLocalIncludeInternal(mainSourceAbspath, quotedName, absPath))
outIncludes.push_back(absPath);
}

if (std::regex_search(line, m, SystemIncludeRe)) {
outIncludes.push_back(m[1].str());
}
}

return true;
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like the collectPrecedingIncludes only scans the immediate parentPath.
What about deeply nested headers?

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.

yes, correct.
collectPrecedingIncludes scans only one level (the immediate parent). This is intentional for this PR, it was designed for (main.cu → A.h → B.h) where B.h depends on explicitly included before it in A.h and collectPrecedingIncludes(A.h, B.h) finds exactly what B.h needs.

I considered collecting full ancestor chain recursively (for nested headers) but that results in over-injection. When two of those injected headers define same symbol, we get redefinition errors.

I would like to take this (deeply nested headers) as a follow-up PR. We track the full ancestors list and modify from {hdr, parentPath} to {hdr, vector<parentChain>}. And only injects that appear in ancestors(parents) up to the nearest file that explicitly includes the current target. This avoids over-injection issue.

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.

#2507: Transitive preceding includes for deeply nested recursive headers

Comment thread src/LocalHeader.cpp Outdated
@ranapratap55
Copy link
Copy Markdown
Collaborator Author

ranapratap55 commented Apr 7, 2026

Follow up work

Comment thread src/LocalHeader.cpp Outdated
Comment thread src/LocalHeader.cpp Outdated
@ranapratap55 ranapratap55 force-pushed the users/ranapratap55/implict-local-headers branch from 8d1113a to 8a1adf5 Compare April 13, 2026 05:54
@ranapratap55 ranapratap55 requested a review from emankov April 13, 2026 06:11
@emankov
Copy link
Copy Markdown
Collaborator

emankov commented Apr 16, 2026

The added tests cover basic recursion and depth-1 injection of “includes”. However, the test that verifies the transitive context is preserved during recursive traversal is still needed.

Please add a test that includes a non-self-contained header with a recursion depth of 2 or more. For example, the root .cu file includes <cmath> and parent.h; parent.h includes child.h; and child.h uses sqrtf but does not include <cmath> itself. This is essential to ensure that the inherited compilation context is correctly propagated down the inclusion chain and is not dropped when processing nested files.

@emankov
Copy link
Copy Markdown
Collaborator

emankov commented Apr 16, 2026

I honestly believe we need to preserve -local-headers and --local-headers-recursive, since the feature you're introducing is experimental and shouldn't be used by default. What do you think, @ranapratap55?

@ranapratap55 ranapratap55 force-pushed the users/ranapratap55/implict-local-headers branch from 8a1adf5 to e198b45 Compare April 20, 2026 10:12
@ranapratap55
Copy link
Copy Markdown
Collaborator Author

The added tests cover basic recursion and depth-1 injection of “includes”. However, the test that verifies the transitive context is preserved during recursive traversal is still needed.

Please add a test that includes a non-self-contained header with a recursion depth of 2 or more. For example, the root .cu file includes <cmath> and parent.h; parent.h includes child.h; and child.h uses sqrtf but does not include <cmath> itself. This is essential to ensure that the inherited compilation context is correctly propagated down the inclusion chain and is not dropped when processing nested files.

I've adapted to use <algorithm> + std::sort

  • transitive_system_include.cu: includes <algorithm> and transitive_parent.h
  • transitive_parent.h: includes transitive_child.h and <cuda_runtime.h>, but no <algorithm>
  • transitive_child.h: uses std::sort and cudaMalloc, includes neither <algorithm> nor <cuda_runtime.h>

collectAncestorSystemIncludes walks the ancestor chain and collects <algorithm> from the main src file, then injects it before compiling transitive_child.h. If this propagation breaks, the test fails directly.

@ranapratap55
Copy link
Copy Markdown
Collaborator Author

ranapratap55 commented Apr 20, 2026

I honestly believe we need to preserve -local-headers and --local-headers-recursive, since the feature you're introducing is experimental and shouldn't be used by default. What do you think, @ranapratap55?

Thanks for raising this and it is a fair concern.

Based on our earlier discussions, we collectively moved toward implicit-by-default to lower the barrier for users. The current revision already has multiple layers in place for end users like

  • Opt-out flag: --skip-local-headers for users who don't want automatic hipification
  • Non-self-contained header warning: [HIPIFY] warning: non-self-contained header is emitted when a header cannot be hipified in isolation and with a note warning the user to run the parent source file instead

And I am thinking to add below

  • Auto-hipification notice: an info-level message [HIPIFY] note: local headers detected: hipifying automatically. Use --skip-local-headers to disable.

With these measures together I believe the implicit behavior is transparent. That said, I am happy to address any specific scenario where this causes harm directly rather than revert the design.

What do you think? @emankov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Feature request or implementation fix It fixes bug partial Partial fix or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants