Fix matcher has_value tolerance for low precision types - #5190
Open
CharlieL7 wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the match::has_value literal matcher to use type-specific tolerance windows so that comparisons remain meaningful for low-precision floating types (notably fp8), while preserving the historical behavior for float. This fits into MIGraphX’s matcher infrastructure by making constant-pattern matching more robust across literal element types.
Changes:
- Introduces per-
shape::type_tdefault tolerance multipliers and refactors thehas_valueimplementation to apply them. - Updates
has_valuecomparison logic to compute the difference indouble, avoiding extra rounding in narrow types. - Adds new matcher tests covering fp8 and bf16 neighbor/rounding cases, and documents the behavior change in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/include/migraphx/matcher.hpp |
Adds per-type default tolerances and refactors has_value matching logic to avoid overly-wide windows for fp8/bf16/half. |
test/matcher.cpp |
Adds regression tests ensuring has_value doesn’t match neighboring fp8/bf16 representable values and preserves float behavior. |
CHANGELOG.md |
Documents the behavioral fix to has_value for narrow types and its downstream impact on passes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1243
to
+1247
| // A literal views const data, so drop the qualifier or numeric_limits will miss the | ||
| // specialization for the narrow types and report an epsilon of zero. | ||
| using type = std::remove_cv_t<typename decltype(v)::value_type>; | ||
| auto eps = static_cast<double>(std::numeric_limits<type>::epsilon()); | ||
| auto window = eps * (atol + rtol * std::fabs(target)); |
pfultz2
reviewed
Aug 25, 2026
Comment on lines
+1185
to
+1186
| double atol_mult = 0; | ||
| double rtol_mult = 0; |
Collaborator
There was a problem hiding this comment.
Can we name this without the _mult postfix:
Suggested change
| double atol_mult = 0; | |
| double rtol_mult = 0; | |
| double atol = 0; | |
| double rtol = 0; |
| /// The window has_value uses when the caller does not name one. Every enumerator is listed so that | ||
| /// a type added to shape::type_t has to name its own window here rather than silently picking up a | ||
| /// window sized for float. | ||
| inline value_tolerance default_value_tolerance(shape::type_t t) |
Collaborator
There was a problem hiding this comment.
Move this into the value_tolerance class and rename it to get_defaults.
Comment on lines
+1234
to
+1235
| optional<double> atol_mult, | ||
| optional<double> rtol_mult) |
Collaborator
There was a problem hiding this comment.
Just pass the value_tolerance. Probably need to make them optional<double> in the struct.
|
|
||
| /// As above with the epsilon multiples given explicitly. Both zero requires an exact match. | ||
| template <class T> | ||
| inline auto has_value(T x, double atol_mult, double rtol_mult) |
Collaborator
There was a problem hiding this comment.
This should take the value_tolerance struct. Since its an aggregate type it will help make it clearer which value is being set: has_value(x, {.atol = 1e-3, .rtol = 1e-3})
Check flagged results 🔆 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
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
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.
Motivation
has_valuematcher uses a set epsilon multiple that only makes sense for fp32 types.Technical Details
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.