Repository navigation
Conversation
✅ 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. |
56a9440 to
2187e6d
Compare
Derive ScalarTag storage from the encoding, so the tag takes only the ScalarType and structured-sparsity code names ScalarTag<ScalarType::UInt8> without restating uint8_t. Document the dispatch pattern in the header and README, and make the forwarding and exception tests state their intent.
c60084e to
72a79b0
Compare
tony-davis
left a comment
There was a problem hiding this comment.
Thanks @newling, this looks good to me. I built the PR head with g++ 15 (-std=c++20 -Wall -Wextra -Wpedantic, ASan and UBSan) and all 6 tests pass with no warnings. I also compiled and ran the README countZeros16 example, and it gives the counts I expected. Approving. I'd like a few small things tidied up.
Please fix the description. I think it has drifted from the code in a few places.
- It says invalid types throw
std::invalid_argument. The code, README, and tests all usestd::runtime_error(scalar_type.hpp:374). - It says the header comment shows the dispatch-once pattern and the sub-byte branch. That example is in the README now, and the header comment is short.
- It says the README "points to" the header. The README now has a full example and a new "Error handling" section, and neither is mentioned. The new
ScalarDispatchSqrttest isn't either.
The new "Error handling" section is a project-wide policy. I think it deserves a line in Technical Details, or its own PR. It also says errors carry a "useful message", but "Invalid ScalarType." doesn't include the bad value. Could we append the numeric value and check for it in ScalarDispatchExceptions?
A few smaller suggestions, take or leave:
- The README
countZeros16example isn't compiled by CI, so it could drift. SinceScalarDispatchSqrtnow serves as the compiled example, maybe the README could link to it and trim its copy. - The README tells readers to name
detail::ScalarTag<...>directly. A public alias would feel nicer than asking people to use adetailname. - The wording moves between "format", "encoding", and "type". The README also says the header documents "reference returns", but the header only says value categories are preserved.
- In
ScalarDispatchTags,Tag::storageBits == scalarTypeInfo(...).storageBitsandis_void == isPacked()come from the same rule as the implementation, so they can't fail on their own. The independent table already covers it, so I'd consider dropping them. - I couldn't see a Windows or MSVC lane that builds HostNumerics (those jobs were skipped). Do you know if one exists? If not, I'd mention it in the Test Result.
- Tiny nit:
typedefatscalar_type.hpp:300could beusing.
One more thing for later, no change needed here: each call site instantiates the visitor once per format (27 times), and nested two-type dispatch would be 729. A line in the header comment about compile cost might help the next person.
Reviewed with the ROCm PR-quality skill.
- Skill:
rocm-pr-quality(base) - overlays:hipblaslt-pr-quality(prose/description rules only; component paths N/A)- Action:
review- Result: APPROVED (0BLOCKING;2IMPORTANT)- Risk:
2- Reviewed commit:
9b38cef2bc5230c32f524827a3221d2936a2aa86- Attestation:
209ebc47Machine-checkable attestation. The digest ties this review to this PR; later commits are
allowed. The reviewed commit is informational (what the review looked at).
|
Thanks for the comments @tony-davis I am addressing them all now.
Will do.
Yes, I'm going to get the README in good shape.
Good point. I'm going to start looking into compile time improvements now. It's about 2 minutes on #10553, but this quadratic dep on types is probably a large factor already |
ScalarTag previously took the storage type as a second template argument, so code naming a tag directly could pair an encoding with the wrong storage, such as ScalarTag<ScalarType::Float16, float>. Derive Storage from the encoding in one place, so ScalarTag<Type> is the only spelling and is the same type that visitScalarType passes.
Explain that visitors name the tag explicitly, recommend explicit return types, and show the dispatch-once pattern with its branch for sub-byte storage. Point to the contract from the README.
Report the visitor overload that ran by name instead of offset integers, explain why the visitor and arguments are move-only, and say why the invalid-type cases check both the call count and the moved argument.
7af4c39 to
48b7e81
Compare
TheRock Submodule Bump ActivityNewest first
|
JIRA ID : AIHPBLAS-4195
Motivation
HostNumerics identifies each supported scalar encoding with
ScalarType, a runtime enum. Element storage, conversion, comparison, and generation code needs that encoding at compile time to select storage types and decoding rules. The numerical implementation needs a shared bridge so each operation does not repeat a switch over all supported encodings.Storage alone cannot identify the encoding.
Float16,BFloat16, andUInt16all useuint16_t, but interpret those bits differently.Technical Details
This PR adds
visitScalarType(type, visitor, args...)toscalar_type.hpp. It calls the visitor's templated call operator with a tag describing the selected encoding:The tag supplies
Tag::type,Tag::storageBits, andTag::Storage. Signed integers,float,double, and complex encodings use native storage types. Other encodings with whole-byte storage use unsigned integers of the same width. If the element width is not divisible by eight bits,Storageisvoid: elements require packed-bit access rather than individually addressable C++ objects. Distinct encodings retain distinct tags even when they share storage.Every visitor is instantiated for every supported encoding and must return the same type for each. Visitors and arguments are forwarded, and value, reference, and
voidreturns are preserved. Callers can dispatch once before an element loop. Invalid enum values throwstd::runtime_errorwith the rejected numeric value before invoking the visitor or moving its arguments; exceptions from the visitor propagate unchanged.The header gives the visitor requirements, and the scalar tests include a compiled vector square-root example. The README provides project orientation and links to build, API, and error-handling documentation. The error-handling document explains the accepted HostNumerics convention: terminal errors owned by the component use
std::runtime_error, expected support failures use explicit results or checks, and allocation, system, dependency, and visitor errors retain their types. This change adds no consumer or default monorepo-build integration.Test Plan
The existing scalar test file checks each dispatched encoding, its independent expected storage type, and a single visitor invocation, along with visitor and argument forwarding, value/reference/void returns, and invalid-type diagnostics without invocation or argument consumption. A templated vector square-root example accepts matching native float/double storage and rejects integer, encoded-half, and mismatched storage. Visitor errors retain their original type and payload.
Test Result
The current local tree passes standalone CTest with GCC 13 and Clang 23 in Release mode and Clang 18 with ASan and UBSan. The Python-enabled Clang 18 build, installed-package consumer, component clang-tidy, and root pre-commit checks also pass. The previously published head passed the standalone hosted CI. HostNumerics standalone CI currently runs only on Ubuntu; Windows/MSVC validation is unavailable. The isolated component retains standalone and repository checks while
ci:skipandproject: nonesuppress unrelated TheRock and Math CI coverage.Submission Checklist
Risk level
Low (2/5): adds scalar dispatch and its contract tests without changing product or GPU execution. No code outside
shared/hostnumerics/uses HostNumerics in this PR's tree.