[AMDGPU] Perf: set force-vector-interleave=8 in JIT pipeline#772
[AMDGPU] Perf: set force-vector-interleave=8 in JIT pipeline#772paveltc wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cb26b9824
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
paveltc
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
paveltc
force-pushed
the
fix/amdgpu-vector-interleave
branch
from
2cb26b9 to
5848ae4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5848ae4e0d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| struct AMDGPUSetLoopInterleavePass | ||
| : public llvm::PassInfoMixin<AMDGPUSetLoopInterleavePass> { |
There was a problem hiding this comment.
AGENTS.md requires minimizing contact area for new/experimental features; this AMDGPU-specific pass is defined in the shared llvm_context_pass.h outside the AMDGPU guard, so every CPU/CUDA LLVM include site now compiles and depends on the new LoopInfo/LoopPassManager code even though only jit_amdgpu.cpp registers it. Please keep the pass private to the AMDGPU JIT file, or at least under QD_WITH_AMDGPU, to avoid expanding the common LLVM surface area for this backend knob.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good call — fixed in the latest push. I moved AMDGPUSetLoopInterleavePass out of the shared llvm_context_pass.h and into an anonymous namespace in jit_amdgpu.cpp (its only user), under the existing QD_WITH_AMDGPU guard, with the LoopInfo/LoopPassManager includes local to that file. The common LLVM header no longer pulls in this backend-specific pass, so CPU/CUDA include sites are unaffected. Pure relocation — the pass logic and pipeline registration are unchanged. Rebuilt clean on the ROCm image (AMDGPU + CPU/CUDA include sites) and re-ran a loop-kernel smoke test on MI300X (exact match); the prior full not slow run still stands since codegen is identical.
hughperkins
added
awaiting-contributor-action
Move AMDGPUSetLoopInterleavePass out of the shared llvm_context_pass.h and into an anonymous namespace in jit_amdgpu.cpp (its only user), under the existing QD_WITH_AMDGPU guard, with the LoopInfo/LoopPassManager includes local to that translation unit. This keeps the common LLVM header free of backend-specific code so CPU/CUDA include sites no longer compile or depend on it. Pure relocation: pass logic and pipeline registration are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
| #include "llvm/Transforms/IPO.h" | ||
| #include "llvm/IR/Instructions.h" | ||
| #include "llvm/IR/IRBuilder.h" | ||
| #include "llvm/IR/Metadata.h" |
There was a problem hiding this comment.
do we still need these header changes?
| #if defined(QD_WITH_AMDGPU) | ||
|
|
||
| namespace { | ||
| // New-PM loop pass: annotates innermost loops with llvm.loop.interleave.count |
There was a problem hiding this comment.
comments should be 120c wrapped, not 80c wrapped
| pb.registerLoopAnalyses(lam); | ||
| pb.crossRegisterProxies(lam, fam, cgam, mam); | ||
|
|
||
| // Annotate innermost loops with interleave-count=8 via IR metadata so the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad6d53631b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pb.registerLoopOptimizerEndEPCallback( | ||
| [](llvm::LoopPassManager &lpm, llvm::OptimizationLevel) { | ||
| lpm.addPass(AMDGPUSetLoopInterleavePass(8)); | ||
| }); |
There was a problem hiding this comment.
When print_kernel_amdgcn is enabled, the earlier dump path builds and runs a separate PassBuilder for module_clone without this callback, while the HSACO path below does get the interleave metadata. That means the emitted quadrants_kernel_amdgcn_*.gcn no longer represents the code being linked for this perf change, which can mislead AMDGPU tuning/debugging; register the same callback on the clone pipeline or share a helper.
Useful? React with 👍 / 👎.
| #include "llvm/Analysis/LoopInfo.h" | ||
| #include "llvm/Transforms/Scalar/LoopPassManager.h" |
There was a problem hiding this comment.
The repo AGENTS.md asks reviewers to flag unnecessary contact area, and these loop-pass headers are only needed by the private AMDGPU pass in jit_amdgpu.cpp where they are already included. Since llvm_context_pass.h is included by common/CPU/CUDA code too, adding these dependencies expands the rebuild and dependency surface for unrelated backends; keep them local to the AMDGPU translation unit.
Useful? React with 👍 / 👎.
|
4 participants
Summary
Sets loop interleave count = 8 for the AMDGPU JIT pipeline. Instead of mutating process-wide LLVM command-line state, a small new-PM loop pass (AMDGPUSetLoopInterleavePass) annotates innermost loops with llvm.loop.interleave.count IR metadata; it's registered via registerLoopOptimizerEndEPCallback in compile_module_to_hsaco, so it's scoped to this compilation pipeline only. Vector interleaving lets the LLVM backend unroll/software-pipeline loops, keeping more functional units busy across the wider CDNA wavefronts. The value 8 matches the recommended setting for MI200/MI300 series. No correctness change.
The pass is defined privately in jit_amdgpu.cpp (anonymous namespace, under QD_WITH_AMDGPU) to keep the shared LLVM headers free of backend-specific code.
Verified with a full not slow test suite run on AMD MI300X with QD_AMDGPU_FORCE_PERMLANE64_FALLBACK=1: 4166 passed, 0 AMD-related failures (1 pre-existing flaky x64 timing test unrelated to this change).
Ported from ROCm#8.