Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Required] Title format: feat(clr,rocr): report signal hints in AQL_EXTENSIONS has a multi-component scope (clr,rocr). ROCm squash-merges, so the PR title becomes the permanent commit title. With 7 ROCr files and 1 CLR file, consider feat(rocr): if ROCr is the primary change, or confirm feat(clr,rocr): is an intentional multi-scope form accepted in this repo. Single-component form (feat(rocr):) is the documented convention.
lmoriche
changed the title
395ae13 to
5933b8b
Compare
|
@cfreeamd (re: the title): Changed to |
rocr: add HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS (1 << 1) to hsa_amd_agent_aql_properties_t, next to HSA_AMD_AQL_PROPERTY_EXT_DISPATCH. GetAqlInfoProperties sets it in HSA_AMD_AGENT_INFO_AQL_EXTENSIONS when SignalHintsSupported(): the CP firmware reports AMD_QUEUE_CAPS_CP_SIGNAL_HINTS, read once from the utility queue's caps. As for EXT_DISPATCH, the enumerator's value is the bit index (setFlag, hsa_flag_isset64), not a mask; the header now documents this for the enum and the attribute. The bit replaces HSA_AMD_AGENT_INFO_SIGNAL_HINTS_SUPPORTED (0xA129, added in 1.36 by #12988), which is removed without an alias: 1.36-1.38 were never in a release, as the version history now says. agent_get_info returns HSA_STATUS_ERROR_INVALID_ARGUMENT for 0xA129, which is left unused. Interface version 1.39. clr: the device reads signal-hint support from AQL_EXTENSIONS with hsa_flag_isset64. With a runtime that does not report the bit, hints stay off. Hints stay off with a profiler tool attached. rocrtst: signal_hints and signal_hint_cost skip unless the agent reports the bit; signal_hints checks that the bit matches the caps the CP reports on a queue of its own.
5933b8b to
2cc65d9
Compare
| setFlag(HSA_AMD_AQL_PROPERTY_EXT_DISPATCH); | ||
|
|
||
| // Fill the HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS | ||
| if (SignalHintsSupported()) |
There was a problem hiding this comment.
[shwetakReviewAgent] automated review on behalf of @shwetagkhatri.
GetAqlInfoProperties now reaches SignalHintsSupported, so the first AQL_EXTENSIONS query submits a barrier-AND and waits with HSA_WAIT_STATE_ACTIVE and an infinite timeout, where the base commit did no GPU work.
| setFlag(HSA_AMD_AQL_PROPERTY_EXT_DISPATCH); | ||
|
|
||
| // Fill the HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS | ||
| if (SignalHintsSupported()) |
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Question] GetAqlInfoProperties is now on the SignalHintsSupported() path, so the first HSA_AMD_AGENT_INFO_AQL_EXTENSIONS query materialises the lazily-created utility queue, submits a barrier-AND and blocks in WaitAcquire(..., UINT64_MAX, HSA_WAIT_STATE_ACTIVE). Previously this attribute did no GPU work, and only the dedicated (now removed) attribute paid that cost — callers who never cared about hints (e.g. generic agent-info dumps such as rocminfo) now pay it too, and a hung/resetting GPU turns an agent-info query into an unbounded busy-wait.
Could the capability be cached at queue-create / agent init (or the wait given a bounded timeout with a fall-back to false) so that an info query can't block on the device? If the on-demand cost is intentional, a note on the attribute doc that the first query may block would help.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
Motivation
ROCr reports the AQL extensions an agent supports in one attribute,
HSA_AMD_AGENT_INFO_AQL_EXTENSIONS, read withhsa_flag_isset64; theext kernel dispatch packet is reported there
(
HSA_AMD_AQL_PROPERTY_EXT_DISPATCH). Completion signal handle hints(#12988, #13273) are an AQL extension of the same kind, consumed by the
CP firmware, but #12988 gave them their own boolean query,
HSA_AMD_AGENT_INFO_SIGNAL_HINTS_SUPPORTED. This reports them as an AQLextension instead and removes that query, so there is one way to ask.
Technical Details
rocr:
hsa_ext_amd.h:HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS = (1 << 1)inhsa_amd_agent_aql_properties_t, next toHSA_AMD_AQL_PROPERTY_EXT_DISPATCH = (1 << 0). Interface version 1.39.GpuAgent::GetAqlInfoPropertiessets it whenSignalHintsSupported():the CP firmware reports
AMD_QUEUE_CAPS_CP_SIGNAL_HINTSin the utilityqueue's
amd_queue_t.caps, probed once (std::call_once) on the firstquery. Reading the result through the accessor is race-free: every
caller returns from
call_onceafter the probe's write, so the firstqueries from several threads all see the same answer. The first
AQL_EXTENSIONSquery on a GPU agent now runs the probe (one emptybarrier-AND on the utility queue), as the removed query did.
setFlag(HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS)and read withhsa_flag_isset64(value, HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS), the enumvalue used as the bit index on both sides, as for
EXT_DISPATCH(thebytes read
04 00 ...for hints,02 00 ...for ext dispatch). Thedoxygen of
hsa_amd_agent_aql_properties_tandHSA_AMD_AGENT_INFO_AQL_EXTENSIONSnow says so: each enumerator's valueis a bit index for
hsa_flag_isset64, not a mask, andvalue[0] & HSA_AMD_AQL_PROPERTY_*tests another bit.HSA_AMD_AGENT_INFO_SIGNAL_HINTS_SUPPORTED(0xA129), addedin interface 1.36 by feat(rocr): add signal handle hints and hsa_amd_signal_batch_create #12988: the enum value, its
GetInfocase(
hsa_agent_get_infonow returnsHSA_STATUS_ERROR_INVALID_ARGUMENTfor it). No deprecated alias: 1.36-1.38 were never in a release, and the
version history says so; the 1.36 line keeps the query with "(removed in
1.39, never released)", and the 1.39 line states that it is removed
without a deprecation period. 0xA129 is left unused, with a comment, so
an old binary that still asks gets an error rather than another
attribute's answer.
hsa_amd_signal_set_hintsandamd_hsa_signal.hnow point to the new bit.
release/bkc/therock-10.3-20261027snapshot (and thehip-version_10.3.*build tags), but in no release branch:
release/therock-10.2is atinterface 1.35, and
release/therock-10.1andrelease/bkc/therock-10.2-20261013do not contain it either. Removingthe query breaks no released API; a binary built against develop
1.36-1.38 headers that uses it gets
INVALID_ARGUMENT(clr treats thatas "no hints").
repository finds no use in rocminfo, rocprofiler-sdk, rocprofiler,
roctracer, rocr-debug-agent, rocdbgapi, the tools or any docs or
CHANGELOG; the users were clr and rocrtst, both changed here, and the
runtimes/mirror below.clr:
Device::populateOCLDeviceConstantsreads signal-hint support fromAQL_EXTENSIONSwithhsa_flag_isset64. With a ROCr that does notreport the bit (including develop before this change), hints stay off.
Hints stay off with a profiler tool attached, as before.
rocrtst:
signal_hints(functional) andsignal_hint_cost(performance)skip unless the agent reports the bit.
signal_hintsalso checks that thebit matches what the CP reports: it runs one barrier on a queue of its own
and compares
AMD_QUEUE_CAPS_CP_SIGNAL_HINTSin that queue's caps withhsa_flag_isset64(AQL_EXTENSIONS, HSA_AMD_AQL_PROPERTY_SIGNAL_HINTS), sosetFlagandhsa_flag_isset64are checked to agree on the bit.Not changed:
runtimes/api-headers/include/hsaand the Rustruntimes/ddi/libhsaprovider mirror the AMD extension at revision 1.36(they do not have 1.37/1.38 either) and answer 0xA129 with
false; theirAQL_EXTENSIONSanswer is all zero, consistent with "no hints". Theydrop 0xA129 when they move to 1.39.
One commit: clr and rocrtst use the enum value the rocr header adds. The
change is mostly ROCr (the API, its runtime and its tests), with one clr
consumer, hence
feat(rocr).Issue Tracking
Test Plan
MI300X with CP firmware that reports AMD_QUEUE_CAPS_CP_SIGNAL_HINTS
(caps bit 2). ROCr, clr and rocrtst built from this branch (based on
develop bee2c57), ROCr and clr from develop bee2c57 (baseline),
against the ROCm 10.1 SDK:
AQL_EXTENSIONSfrom 8 threads at once (thefirst, probing queries), reads both flags with
hsa_flag_isset64, andchecks that 0xA129 returns
INVALID_ARGUMENT.rocrtstFunc.Signal_Hints_*androcrtstPerf.Signal_Hint_Cost;the functional tests also on develop's ROCr (a ROCr whose bit does not
match the caps: the negative control of the new check).
AMD_LOG_LEVEL=3, "Signal hints N (profiler tool: N)"):this branch; develop; this branch's clr on develop's ROCr; this branch
under
rocprofv3 --kernel-trace.Performance_PyFR_Couette_StepSimulation, 3 runs each,alternating this branch and develop.
Test Result
AQL_EXTENSIONS=04 00 00 00 00 00 00 00(SIGNAL_HINTS1,EXT_DISPATCH0); 0xA129 returnsINVALID_ARGUMENT.Signal_Hints_*tests (with the bit-vs-caps check) andSignal_Hint_Costpass on this branch (run, not skipped). On develop'sROCr the bit-vs-caps check fails as intended (caps bit 2 set, the bit not
reported), and the hint subtests skip.
"Signal hints 0" with this branch's clr on develop's ROCr and under
rocprofv3 (profiler tool: 1).
total), 0.477-0.478 ms on develop (954.4-955.9 ms).
Submission Checklist