[0035] Address a swath of feedback #759
Conversation
This should remove the need for the annotate matrix calls by mangling the type annotations into the handle.
* Added transpose compile-time argument to `cast` (Resolves microsoft#653). * Renamed `Accumulate` operations that operate on memory to `InterlockedAccumulate`, and clarify atomicity (Resolves microsoft#744). * Replaced `SumAccumulate` with new `Accumulate` on A and B matrices (Resolves microsoft#755, Resolves microsoft#618). * Layout for `Matrix::Load` on thread-scope matrices is now compile-time constant (Resolves microsoft#681). * `InterlockedAccumulate` for thead-scope matrices no longer takes a layout parameter since it must be OuterProductOptimal. * Added clarification of matrix stage availability (Resolves microsoft#574). * Fixed the form of the outerProduct DXIL op to not create a matrix (Resolves microsoft#696).
|
Since this PR is a larger collection of fixups I'll throw anoter comment related to the spec that I noticed that should be quick to fix or reject! Should hlsl-specs/proposals/0035-linalg-matrix.md Line 1176 in 65815b0 |
| typename hlsl::enable_if<Use == MatrixUse::Accumulator && ScopeLocal == Scope, | ||
|
|
||
| // When Scope != Thread, the following overloads are available: | ||
| template <MatrixUseEnum UseLocal = Use> |
There was a problem hiding this comment.
I don't understand this template parameter UseLocal. It's not used except to make sure it's equal to Use (the default) which is required to be equal to MatrixUse::Accumulator. I see it on a number of methods where it's unused. Is this to satisfy some requirement elsewhere?
There was a problem hiding this comment.
It's how SFINAE works (see: #590 (comment)).
Co-authored-by: Ashley Coleman <ascoleman@microsoft.com>
Co-authored-by: Ashley Coleman <ascoleman@microsoft.com>
4 participants
This PR folds in a bunch of changes to address feedback that has been backed up. One big change is the removal of
annotateMatrixin favor of type mangling. Using type mangling reduces the need for the compiler to emit and de-duplicateannotateMatrixcalls, and aligns more closely to how we'll support this in modern LLVM using target extension types. This is a first step to addressing matrix lifetime issues.AttributedMatrixRefcast(Resolves [0035] Add transpose to cast #653).Accumulateoperations that operate on memory toInterlockedAccumulate, and clarify atomicity (Resolves [0035] Need to specifyMatrix::Accumulateis an atomic operation #744).SumAccumulatewith newAccumulateon A and B matrices (Resolves [0035] Consider removing SumAccumulate #755, Resolves [0035] SumAccumulate matrix operand dimensions and types #618).Matrix::Loadon thread-scope matrices is now compile-time constant (Resolves [0035] For thread scope matrices, make layout of matrix in Matrix::load a compile-time constant #681).InterlockedAccumulatefor thead-scope matrices no longer takes a layout parameter since it must be OuterProductOptimal.Strideparameters (partial feedback from [0035] Address feedback from David Neto #581).