Skip to content

fix(miopen): Remove direct __ocml calls - #13312

Merged
arsenm merged 1 commit into
developfrom
users/arsenm/miopen-remove-ocml-calls
Oct 9, 2026
Merged

arsenm merged 1 commit into
developfrom
users/arsenm/miopen-remove-ocml-calls

Conversation

@arsenm

@arsenm arsenm commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

These are compiler implementation details which shouldn't be exported, much less used by external users. Prefer using the HIP public math functions. In the case of fmin/fmax, use the clang/gcc builtin since presently the HIP header is open coding a poor implementation of these functions.

exp and log are open coded with __builtin_amdgcn_exp2f/__builtin_amdgcn_logf, matching the existing __ocml_exp_f16/__ocml_log_f16 expansions. These could migrate to the standard functions as well, though without identical codegen (I would also consider providing a pow that's simply implemented this way ill-advised).

It also appears these half functions aren't actually used, so another fix may be to just drop them.

Generated ISA is unchanged for the suite of targets.

These are compiler implementation details which shouldn't be exported,
much less used by external users. Prefer using the HIP public math functions.
In the case of fmin/fmax, use the clang/gcc builtin since presently the HIP
header is open coding a poor implementation of these functions.

exp and log are open coded with __builtin_amdgcn_exp2f/__builtin_amdgcn_logf,
matching the existing __ocml_exp_f16/__ocml_log_f16 expansions. These could
migrate to the standard functions as well, though without identical codegen
(I would also consider providing a pow that's simply implemented this way
ill-advised).

It also appears these half functions aren't actually used, so another fix may
be to just drop them.

Generated ISA is unchanged for the suite of targets.
@arsenm
arsenm requested a review from a team as a code owner October 8, 2026 13:57
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
📝 PR Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
• JIRA ID : TESTAUTO-6039
• JIRA ID - #330
• JIRA ID #330
• JIRA ID (on separate line)
ROCM-25757
• ISSUE ID : TESTUTO-3334
• ISSUE ID #3334
• ISSUE ID - TESTAUTO-3433
• ISSUE ID (on separate line)
AIRUNTIME-2352
• ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
• Closes #10
• Fixes octo-org/octo-repo#100
• Resolves: #123
• #123
• https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/miopen/src/kernels/miopen_math.hpp; no test file found
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@arsenm

arsenm commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR. Once all policies pass, the label will be removed automatically.

No, this policy is horrible and hostile to both contributors and maintainers.

@averinevg averinevg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@arsenm
arsenm merged commit be3dcc9 into develop Oct 9, 2026
200 of 220 checks passed
@arsenm
arsenm deleted the users/arsenm/miopen-remove-ocml-calls branch October 9, 2026 10:08
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

mattwill-amd pushed a commit that referenced this pull request Oct 14, 2026
JIRA ID: ALMIOPEN-2913

## Motivation

Only 5 of the 30 commits that landed against `projects/miopen` since the
3.6.2 version bump (`2d20a8f775e`, #12211) were reflected in
`CHANGELOG.md`. The remaining 25 were reviewed and the 15
externally-visible ones documented, so the ROCm 10.2.0 section
accurately reflects what shipped.

14 entries added (two of the fixes share one entry), plus a
duplicate-heading fix.

### Added
* [Conv] NHWC (channel-last) support in the GEMM convolution solvers,
with new im2col/col2im kernels (#8693)
* [Conv] Cross-architecture AI heuristics for immediate mode
(`MIOPEN_FIND_MODE=2`), extending data-driven selection to Navi (#9680)
* [Conv] Missing 3x3 stride-1 pad-1 tile-28 CK depthwise backward-data
instance — ALMIOPEN-2423 (#11271)

### Changed
* [Conv] `ConvHipConv` held out of the AI perf-config picker until its
model is retrained — ALMIOPEN-2494 (#13337)
* [Conv] CK byte-span gate lifted for forward and backward-data, now
that CK fixes the 32-bit offset overflow itself — ALMIOPEN-2645 (#12396)

### Optimized
* [Conv] `ConvDirectNaiveConv`: per-direction kernel files, float
accumulators, spatial tiling, bfloat16 path — ALMIOPEN-2485/2486 (#7529)
* Kernel cache reuses cached SQLite connections instead of reopening per
load/store — ALMIOPEN-2598 (#11933)

### Resolved Issues
* [Conv] Incorrect backward-weights results from the gfx950 direct
kernel at `waves_q == 1` (missing cross-lane ordering on LDS staging
reuse) — ROCM-31508 (#12406)
* [Conv] Up to 8x slower hipconv `direct`/`direct_l1`/`direct_wgrad`
under ROCm 10.1's full AMDGCN LTO default; fixed with `-fno-offload-lto`
— ROCM-31334 (#12328)
* [Conv] gfx942 multi-node training throughput regression from backward
GEMM on small-batch 1x1 NHWC shapes — ROCM-31155 (#13232)
* [Conv] Invalid CK perf-config entries in the gfx1201 and gfx1100
system databases — ROCM-29026 (#12088, #12117)
* [BatchNorm] JIT compilation failures on non-GFX9 targets; Welford DPP
assembly restricted to GFX9 — ALMIOPEN-2838 (#12889)
* Out-of-bounds write in `miopenLoadSolution` when a serialized solution
carries more than three work dimensions — ROCM-31082 (#12242)
* `uint64_t` typedef conflict between `miopen_cstdint.hpp` and system
`<stdint.h>` on LP64 Linux (#12623)

## Technical Details

Single-file change to `projects/miopen/CHANGELOG.md`.

The 10.2.0 section had **two** `### Resolved Issues` headings. The two
entries under the trailing one (`ConvHipConv` gfx1250 crash/LDS race,
and the perf-db reload config loss) were merged into the first, and the
trailing heading removed — this accounts for the 2 removed bullets in
the diff. Section order is now `Resolved Issues → Added → Changed →
Optimized → Removed`, matching the 3.6.1 section. `Optimized` is new to
this release section.

Already documented, so untouched: #12874 (reduce int32 rejection),
#12000 / #13016 / #13213 (hipconv v0.3.1 / v0.3.2 / v0.4.0), #11761
(gfx803 removal).

## Notes

Deliberately excluded as not externally visible (10 commits):

* **hipDNN convolution forwarding (#12276)** —
`MIOPEN_ENABLE_HIPDNN_WRAPPER` defaults to `OFF`
(`projects/miopen/CMakeLists.txt:162`), so no shipped build compiles the
forwarding code and the runtime `MIOPEN_HIPDNN_FORWARDING` flag has
nothing to enable.
* **Test-only**: #13289 (gfx1250 depthwise test applicability,
ALMIOPEN-2841), #11610 (hipDNN parity harness), #12660 (Jenkins status
post).
* **Internal kernel-math cleanup, generated ISA unchanged**: #13312
(drop direct `__ocml` calls), #13190 (remove broken single-argument
`__half` overloads).
* **Add/revert pairs, net no change**: #12466 + #13066 (gfx1101 Navi
system DBs), #12496 + #12800 (gfx1250 in the dev image arch lists).

## Risk Assessment

Documentation-only change to a single Markdown file; no code, build, or
runtime impact.

**Risk Level: 🟢 Low**

## Test Plan

None required — Markdown-only. Verified that the entry topics are not
already documented elsewhere in the file, that no content was lost when
merging the duplicate heading, and that every new entry maps to a commit
in `2d20a8f775e..origin/develop -- projects/miopen`.

## Test Result

16 bullets added, 2 relocated — 14 net-new entries covering 15 commits.
Duplicate heading removed; section ordering matches 3.6.1.

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants