Skip to content

Add support for gfx1250 - #4975

Merged
dayatsin-amd merged 33 commits into
developfrom
users/dayatsin/add-gfx1250
Apr 15, 2026
Merged

dayatsin-amd merged 33 commits into
developfrom
users/dayatsin/add-gfx1250

Conversation

@dayatsin-amd

@dayatsin-amd dayatsin-amd commented Apr 13, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Add support for gfx1250 GPU

Test Plan

Tested that these patches pass ROCRtst and HIP catch tests on existing GPUs.

Test Result

Pass

Submission Checklist

@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch 3 times, most recently from 2e201ac to 8b186dc Compare April 13, 2026 18:35
@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch from 8b186dc to c924e14 Compare April 13, 2026 20:09
@dayatsin-amd
dayatsin-amd requested review from a team as code owners April 13, 2026 20:09
@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch 2 times, most recently from 589c93e to 87ed09d Compare April 13, 2026 20:57
@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch from 87ed09d to 9276ce1 Compare April 13, 2026 21:25
@kentrussell

Copy link
Copy Markdown
Contributor

Nothing worth blocking for. Just 2 kfdtest changes and an unneeded capitalization on a hex value. Ignore the hex change unless you have to do another upload anyways. kfdtest changes can be made after these land

@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch 2 times, most recently from a7be5a6 to f601a32 Compare April 13, 2026 23:03
@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/add-gfx1250 branch from 5490a73 to d98e1a2 Compare April 14, 2026 21:36
@randomizedcoder

Copy link
Copy Markdown
Contributor

@dayatsin-amd

As you may have seen from some of my recent PRs, I've been running static analysis tools across the codebase.

One minor thing flawfinder flagged (pre-existing, not introduced by your PR): PrefetchRange in runtime.h L732-739 — the 2-arg constructor doesn't initialize prev and next. The prefetch_lock_ mutex likely protects against any read-before-write in practice, but initializing them in the constructor would make it safe by construction:

PrefetchRange(size_t Bytes, PrefetchOp* Op) : bytes(Bytes), op(Op), prev{}, next{} {}

Not a blocker at all — just a small defensive improvement. Feel free to ignore if it's out of scope for this PR.

@randomizedcoder

Copy link
Copy Markdown
Contributor

You are also touching fmm.c which has some tiny printf cppcheck issues
#5049

@dayatsin-amd

dayatsin-amd commented Apr 14, 2026 •

Copy link
Copy Markdown
Contributor Author

@dayatsin-amd

As you may have seen from some of my recent PRs, I've been running static analysis tools across the codebase.

One minor thing flawfinder flagged (pre-existing, not introduced by your PR): PrefetchRange in runtime.h L732-739 — the 2-arg constructor doesn't initialize prev and next. The prefetch_lock_ mutex likely protects against any read-before-write in practice, but initializing them in the constructor would make it safe by construction:

PrefetchRange(size_t Bytes, PrefetchOp* Op) : bytes(Bytes), op(Op), prev{}, next{} {}

Not a blocker at all — just a small defensive improvement. Feel free to ignore if it's out of scope for this PR.

Please keep PR's focused on a single topic. I will address that change in another PR.

@randomizedcoder

Copy link
Copy Markdown
Contributor

Filed #5051 as many of the files in this PR use the obsolete 'alloca'

@randomizedcoder

Copy link
Copy Markdown
Contributor

Another PR on signal.cpp: #5052

@randomizedcoder

Copy link
Copy Markdown
Contributor

This PR touches amd_hsa_code.cpp which also has memory corruption bug, although it's possibly unused code
#5059

@dayatsin-amd
dayatsin-amd merged commit 34f2e00 into develop Apr 15, 2026
70 of 76 checks passed
@dayatsin-amd
dayatsin-amd deleted the users/dayatsin/add-gfx1250 branch April 15, 2026 04:09
@lumachad

lumachad commented Apr 21, 2026 •

Copy link
Copy Markdown
Contributor

@dayatsin-amd FYI, this broke gfx1201 debugging on rocgdb.

Edit: More specifically this commit: 0853edd

@lancesix

Copy link
Copy Markdown
Contributor

@dayatsin-amd FYI, this broke gfx1201 debugging on rocgdb.

Hi @dayatsin-amd, since this got pinned down to trap handler, I'll try to have a look shortly.

dayatsin-amd added a commit that referenced this pull request May 4, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request May 7, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request May 28, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request Jun 26, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request Jul 6, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request Jul 7, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request Jul 11, 2026
Fix incorreect grid dimensions introduced by:
#4975
dayatsin-amd added a commit that referenced this pull request Jul 11, 2026
## Summary

Fix incorrect grid dimensions introduced by
#4975.

ROCr changes (`amd_gpu_agent.cpp`):
- Set default `kern_cluster_max_dim_` to `{ INT32_MAX, UINT16_MAX,
UINT16_MAX }`
- Remove gfx12-specific override that reported `UINT32_MAX` for the X
dimension
- Clamp `HSA_AGENT_INFO_GRID_MAX_DIM` / `HSA_AGENT_INFO_GRID_MAX_SIZE`
to `INT32_MAX` / `UINT16_MAX`

rocprofiler-sdk companion fix (`platform/gnulinux/agent.cpp`):
- Update hardcoded `grid_max_size` from `UINT32_MAX` to `INT32_MAX` so
agent info matches ROCr

## JIRA ID

JIRA ID : ROCM-23909

## Test plan

- [ ] Run rocminfo and verify grid max dimensions
- [ ] `rocprofiler_sdk.unit.rocprofiler_lib.agent`

---------

Co-authored-by: Cursor <cursoragent@cursor.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.