fix(frontend): stop autotune demanding variant pack entries for value-carrying scalars - #11506
Open
SamuelReeder wants to merge 1 commit into
Open
fix(frontend): stop autotune demanding variant pack entries for value-carrying scalars#11506SamuelReeder wants to merge 1 commit into
SamuelReeder wants to merge 1 commit into
Conversation
autotuneImpl() requires the variantPack to carry a UID for every
non-virtual tensor. A scalar that carries a baked value - a compile-time
constant, or a runtime-with-default - reaches the provider through the
op-graph flatbuffer, not the variantPack (RFC 0016 section 2.2), so it
has no pointer to give. execute() accepts a pack that omits it;
autotune() rejects the same pack, which makes any graph containing a
baked scalar impossible to autotune.
Observed on gfx90a through the Python frontend:
batchnorm_training -> missing required ... UIDs: 4, 7
(epsilon, momentum)
rmsnorm -> missing required ... UIDs: 3 (epsilon)
batchnorm_inference_variance -> missing required ... UIDs: 6 (epsilon)
The requirement is also unsatisfiable for the runtime-with-default kind:
RFC 0016 section 2.2 states an attempted variantPack override of one
errors, so the caller can neither omit the entry nor supply it.
Exempt the two value-carrying kinds. get_is_pass_by_value() is not the
right predicate: it is _isRuntimePassByValue || hasValue(), true also for
a runtime user-supplied scalar, which carries no value and *is* delivered
through the variantPack as a host pointer. get_has_compile_time_constant()
alone is not sufficient either: it is !_isRuntimePassByValue && hasValue(),
so it misses the runtime-with-default kind whose value is equally baked.
The two terms are disjoint and their union is exactly hasValue(), which
is private.
The check itself is kept. It fails fast with a precise message before a
sweep compiles and benchmarks many plans; without it an incomplete pack
reaches the backend as a missing pointer. Only its predicate was wrong.
Adds three tests on a batchnorm graph with an epsilon scalar, one per
state, mirroring the graphs that failed in practice:
...ForCompileTimeConstantScalars (!runtime, value) -> exempt
...ForRuntimeDefaultScalars ( runtime, value) -> exempt
...ForRuntimeUserSuppliedScalars ( runtime, novalue) -> required
Each term is load-bearing; rebuilding the validator against four
predicates fails a distinct test:
unpatched (no exemption) first two FAIL
get_is_pass_by_value() (broad) third FAILS
compile-time constant term only second FAILS
this change all pass
Existing coverage is untouched: AutotuneRejectsMissingTensorUids,
AutotuneAcceptsCompleteVariantPack and AutotuneAcceptsExtraUidsInVariantPack
use no value-carrying tensors and pass unmodified.
Test: hipdnn_frontend_tests, 2123 tests from 129 suites, all pass.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
SamuelReeder
marked this pull request as ready for review
August 31, 2026 20:39
SamuelReeder
changed the title
BrianHarrisonAMD
approved these changes
Aug 31, 2026
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks for the fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #11506 +/- ##
===========================================
+ Coverage 69.72% 69.72% +0.01%
===========================================
Files 2810 2811 +1
Lines 464097 464154 +57
Branches 68444 68447 +3
===========================================
+ Hits 323551 323625 +74
+ Misses 117028 117017 -11
+ Partials 23518 23512 -6
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
autotuneImpl()requires the variantPack to carry a UID for every non-virtual tensor. A scalar that carries a baked value — a compile-time constant, or a runtime-with-default — reaches the provider through the op-graph flatbuffer, not the variantPack (RFC 0016 §2.2), so it has no pointer to give.execute()accepts a pack that omits it;autotune()rejects the same pack. Any graph containing a baked scalar is therefore impossible to autotune.JIRA ID : ALMIOPEN-2462
Risk Assessment
Risk level: 2 (low). The change narrows one predicate to exempt a tensor class that provably has no pointer to supply, matching the pack shape
execute()already accepts. It cannot mask a genuinely absent operand: non-virtual tensors that carry no baked value — including runtime user-supplied scalars, which are variantPack-delivered as host pointers — remain required, and a dedicated test pins that. The blast radius isautotuneImpl()'s pre-flight check; no execution, compilation, or ranking path changes. The main residual risk is a provider that expects a pointer for a tensor carrying a baked value, which would contradict RFC 0016 §2.2 and would already be broken underexecute().ASIC Coverage
Passing PR CI is sufficient — no specific-ASIC run required. The change is host-side graph validation with no kernel, code-object, or arch-dependent behavior: it reads
TensorAttributesmetadata and decides whether to append a UID to an error list. Behavior is identical across all GFX families. The functional evidence below was nonetheless collected on gfx90a (MI210).Testing Summary
Before the fix, five engine rows across three op classes could not be tuned:
After the fix, the same sweep over 21 sample graphs tunes 22 of 22 applicable rows with 0 errors (previously 17 of 22 with 5 errors). Correctness results are unchanged.
Writing
Rfor_isRuntimePassByValueandVforhasValue(), the three states and the two getters that discriminate them:get_has_compile_time_constant()get_pass_by_value().has_value()Both terms are load-bearing. Rebuilding the validator against four predicates fails a distinct test each time:
...CompileTimeConstantScalars...RuntimeDefaultScalars...RuntimeUserSuppliedScalarsget_is_pass_by_value()Row 2 is why the predicate is not
get_is_pass_by_value(): that getter isR || V, true also for the runtime user-supplied kind, which must appear in the pack. Row 3 is why one term does not suffice:get_has_compile_time_constant()is!R && V, so it misses runtime-with-default, whose value is equally baked.Testing Checklist
./bin/hipdnn_frontend_tests- Status: Passed (2123 tests from 129 suites)--gtest_filter="TestGraph.Autotune*Scalars"- Status: Passed (3/3)clang-format --dry-run --Werroron both changed files - Status: PassedTechnical Changes
projects/hipdnn/frontend/include/hipdnn_frontend/Graph.hppThe variantPack completeness check inside
autotuneImpl()gains two predicates:The two terms are disjoint (
!R && VandR && V) and their union is exactly the privatehasValue(), spelled through public getters.The check itself is deliberately kept. It fails fast with a precise message before a sweep compiles and benchmarks many plans; without it an incomplete pack reaches
populateBaseVariantPackDescriptor, which copies the map verbatim with no notion of a required tensor, and the failure surfaces backend-side as a missing pointer. Only the predicate was wrong. Removing the block entirely — restoring symmetry withexecute(), which performs no completeness check at all — is a defensible but separate design decision, and would delete the three existing tests under "Autotune UID Validation Tests"; I did not take it.projects/hipdnn/frontend/tests/TestGraph.cppAdds
AutotuneDoesNotRequireUidsForCompileTimeConstantScalars,AutotuneDoesNotRequireUidsForRuntimeDefaultScalarsandAutotuneStillRequiresUidsForRuntimeUserSuppliedScalarsbeside the existing autotune UID-validation tests, sharing a smallcreateBatchnormGraphWithEpsilonhelper. All three build abatchnormgraph with anepsilonscalar, mirroring the graphs that failed in practice, and differ only in epsilon's state.Notes for reviewers
Two incidental observations, neither addressed here:
tensor->has_uid()term in this loop appears vestigial.lowerGraphToDescriptors()callsassignUnsetTensorUids()before validation, and this check runs only afterhasReadyGraphDesc()passes, so every tensor already has a UID by then. Pre-existing; left alone to keep the diff minimal.execute()would restore symmetry, butgatherHipdnnTensorsSubtree()builds a hash set over the whole graph on every call, which would land inside the timed region of any benchmark loop. If it is ever wanted, computing the required-UID set once at build time and caching it would avoid the per-execute traversal.