[AMDGPU] Fix: apply amdgpu-ieee/amdgpu-dx10-clamp to all functions - #775
[AMDGPU] Fix: apply amdgpu-ieee/amdgpu-dx10-clamp to all functions#775paveltc wants to merge 1 commit into
Conversation
paveltc
changed the title
|
Note Outdated (Aug 20, 2026): This comment no longer applies to the current PR. Keeping it for historical reference. Depends on #774 — please review/merge that first. This branch is stacked on #774: of its three commits, the first two ( The actual change here is a single-file, +16/-6 edit to Plan: once #774 merges, I'll rebase this onto |
|
@codex review |
![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: 23f2bb97b7
ℹ️ 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".
| if (this->config_.fast_math) { | ||
| F.addFnAttr("unsafe-fp-math", "true"); | ||
| F.addFnAttr("no-signed-zeros-fp-math", "true"); |
There was a problem hiding this comment.
The stated inlining fix only requires propagating amdgpu-ieee and amdgpu-dx10-clamp, but this commit also changes fast-math attributes, scheduling and work-group heuristics, DAZ behavior, cache keys, bindings, and the public kernel API. These independent semantic changes greatly increase the regression surface of the backport; please partition them into separate changes and keep this fix localized to the two attributes.
AGENTS.md reference: AGENTS.md:L7-L13
Useful? React with 👍 / 👎.
| fastcache: bool = False, | ||
| graph: bool = False, | ||
| checkpoints: bool = False, | ||
| cuda_graph: bool = False, |
There was a problem hiding this comment.
This adds cuda_graph to the public @qd.kernel API and gives it deprecation and graph-enabling behavior, but neither the updated user guide nor this function's Args section documents it. Users therefore cannot discover its semantics or migration path from the public documentation; add the corresponding docs/ update if this alias remains.
AGENTS.md reference: AGENTS.md:L15-L22
Useful? React with 👍 / 👎.
| "amdgpu-max-num-workgroups", | ||
| "amdgpu-agpr-alloc", | ||
| "amdgpu-waves-per-eu", | ||
| "amdgpu-flat-work-group-size", | ||
| "amdgpu-sched-strategy", |
There was a problem hiding this comment.
When a user tries to override amdgpu-ieee or amdgpu-dx10-clamp through fn_attrs, _validate_fn_attrs rejects the decorator because neither name is registered here. This makes the new JIT hasFnAttribute guards—and the documented claim that user-supplied values win—unreachable for the two attributes central to this change; register both names or remove the unsupported override claim.
Useful? React with 👍 / 👎.
These two attributes are applied to all functions in jit_amdgpu.cpp behind `!F.hasFnAttribute(key)` guards, and the PR documents that a user-supplied value via @qd.kernel(fn_attrs=...) wins over the default. But neither name was in the fn_attrs registry, so _validate_fn_attrs rejected any attempt to override them, making that override path unreachable (Codex Genesis-Embodied-AI#775 P2). Register both names so the guards are actually reachable. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks @codex. Quick disposition of the three comments: P1 — "Split unrelated AMDGPU behavior out of this fix" ( Both of these are from the inherited #774 commits, not this PR's actual change. This branch is stacked on #774: of its three commits, the first two ( So the fix is already localized to the two attributes as requested; the extra surface Codex is flagging lives in #774. Once #774 merges, I'll rebase this onto P2 — "Allow the guarded AMDGPU attributes through validation" ( Good catch — this one is real and specific to this PR's claim. Fixed in |
|
To use Codex here, create a Codex account and connect to github. |
Set amdgpu-ieee=false and amdgpu-dx10-clamp=false on all functions in the AMDGPU module rather than only AMDGPU_KERNEL entries. LLVM's inliner refuses to inline a callee into a caller when they carry mismatching target-specific attributes. Applying these attributes to kernels only leaves internal runtime device functions (e.g. gpu_parallel_range_for and the body functions it dispatches) with a mismatching attribute set, so they are not inlined into the kernel entry. Without that inlining, InferAddressSpaces cannot follow the pointer chain from kernel parameters to field data and cannot promote flat_load/flat_store/flat_atomic to global_*, causing flat-atomic coherency issues and a ~4% throughput regression on gfx942 (MI300X). Applying the two attributes uniformly restores inlining and lets InferAddressSpaces emit global_load/global_store/global_atomic. Each write is guarded by hasFnAttribute so it remains idempotent. Self-contained backend change: single file, no public API, no dependency on the per-kernel fn_attrs work in Genesis-Embodied-AI#774. Co-authored-by: Cursor <cursoragent@cursor.com>
d62feed to
99a4eba
Compare
paveltc
changed the title
2 participants
Summary
Backend-only AMDGPU codegen fix. No public API, no new user-tunable knobs, solver-agnostic. This PR is self-contained and does not depend on #774.
On gfx942 (MI300X),
amdgpu-ieee=falseandamdgpu-dx10-clamp=falsemust be applied to all functions, not onlyAMDGPU_KERNELentries. LLVM's inliner refuses to inline a callee into a caller when they carry mismatching target-specific attributes, so applying them kernel-only leaves internal device functions (e.g.gpu_parallel_range_forand the body functions it dispatches) un-inlined. Without that inlining,InferAddressSpacescannot follow the pointer chain from kernel parameters to field data and cannot promoteflat_load/flat_store/flat_atomictoglobal_*, causing flat-atomic coherency issues and a ~4% throughput regression on MI300X.Change
Single file (
quadrants/runtime/amdgpu/jit_amdgpu.cpp): a small pass setting both attributes on every function, each guarded by!F.hasFnAttribute(...)for idempotency. +21 lines, no other files touched.Independence from #774
An earlier version of this fix was stacked on #774 and reused its per-kernel
fn_attrsregistry. That coupling has been removed — this PR now applies againstmaindirectly as a self-contained backend change and can merge with or without #774.