GPU data tiling on RDNA3 #18980
Merged
GPU data tiling on RDNA3 #18980
Conversation
This file contains 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
bjacob
force-pushed
the
users/bjacob/rdna3-data-tiling
branch
from
06542f5 to
aec9f1b
Compare
bjacob
force-pushed
the
users/bjacob/rdna3-data-tiling
branch
from
aec9f1b to
9351397
Compare
Groverkss
approved these changes
Nov 7, 2024
compiler/src/iree/compiler/Codegen/Common/GPU/test/CMakeLists.txt
Outdated
Show resolved
Hide resolved
| // only to the layout dims, and do not reflect a possible additional | ||
| // thread-distribution-only dimension present on some architectures (RDNA3). | ||
| // When such an extra dim exists, multiple threads are reading the same data. | ||
| // So we need to distinguish layoutThreadSizes vs. distributionThreadSizes. |
There was a problem hiding this comment.
Do you think you could put an example of how this looks like for an intrinsic? This might not be obvious to someone who hasn't stared at RDNA3 intrinsics enough.
There was a problem hiding this comment.
Actually I just expanded the general explanation, because I still felt that this was something that we are doing from first principles and can explain a such. I felt that to really work out an example would take more space than we could spend here.
Comment on lines
980
to
986
| // Most of the rest of this function is the computation of the offsets. | ||
| // The basic idea is to delinearize the threadId over the basis of | ||
| // cross-thread dimensions. The main subtlety is that the TileSwizzles refer | ||
| // only to the layout dims, and do not reflect a possible additional | ||
| // thread-distribution-only dimension present on some architectures (RDNA3). | ||
| // When such an extra dim exists, multiple threads are reading the same data. | ||
| // So we need to distinguish layoutThreadSizes vs. distributionThreadSizes. |
There was a problem hiding this comment.
So if we have an intrinsic that takes let's say 32 elements, we are essentially broadcasting it to size 64, and distributing it. And when doing anything else, we ignore the broadcasted dimension, to get the same data on different threads.
Comment on lines
1032
to
1036
| // Erase the delinearized index that corresponds to the extra distribution | ||
| // dimension that we had inserted above. | ||
| tileOffsets.erase(tileOffsets.begin() + distributionOnlyDimIdx); |
There was a problem hiding this comment.
Can you mention that this essentially leads to threads reading the same data, accomplishing what we wanted for the intrinsic.
Signed-off-by: Benoit Jacob <[email protected]>
Signed-off-by: Benoit Jacob <[email protected]>
bjacob
force-pushed
the
users/bjacob/rdna3-data-tiling
branch
from
ce464c9 to
04bc2d2
Compare
bjacob
added a commit
that referenced
this pull request
Nov 16, 2024
Working on #18980 let me spend quality time with e2e matmul tests and suggested some changes. The main change is to simplify the printing of numerical values to always use high precision, meaning print all significant digits of floating point values. Since our tests generate small integral values and the intent is generally to be testing mostly the exact arithmetic that happens on small integral values, in most cases this doesn't make any difference. But I found that RDNA3 float arithmetic produces non-exact results even on those values. As a result, I got values like 1+epsilon where 1 was expected, causing a test to fail (since we didn't know we needed to opt out from requiring exact results) and the test output cryptically printed both values as "1". The other change is to more consistently print the same number of rows and columns regardless of whether we are at the start or in the middle of a dimension, and to have that number be what we call "context" (before, it was "2 * context"). Also a seasonal emoji change. Signed-off-by: Benoit Jacob <[email protected]>
Groverkss
pushed a commit
to Groverkss/iree
that referenced
this pull request
Dec 1, 2024
A few things were needed: * Populate required fields in KnownTargets.cpp. * Support the case where the intrinsic vector operand size is greater than the load instruction size (here it is 16xf16 = 256 bit). * `buildMmaOperation` creates `vector.insert_strided_slice` to insert the new accumulator vectors into the accumulator tile. In doing so, it was relying on `vector.insert_strided_slice` implicit expand-shape semantics, in ways that worked for the shapes we had seen in CDNA3 but not here. Solved by explicitly expanding shapes with `vector.shape_cast` ops. * In thread-distribution code (populateOperandXxx), we needed to account for the nuance between two distinct thread grids: "layout" vs "distribution". In the case of RDNA3, there is a distribution-only dimension that isn't reflected in the layout-centric TileSwizzle's. * On RDNA3, some float arithmetic is strongly non-IEEE754-compliant: even exactly-representable small integral values, on which float arithmetic should be exact, have epsilon numerical errors! Addressed by tolerance. * Fix a bug: the doubly-nullable type `std::optional<IREE::GPU::DataTiledMMAAttr>` tricked us, change to `IREE::GPU::DataTiledMMAAttr`. --------- Signed-off-by: Benoit Jacob <[email protected]>
Groverkss
pushed a commit
to Groverkss/iree
that referenced
this pull request
Dec 1, 2024
Working on iree-org#18980 let me spend quality time with e2e matmul tests and suggested some changes. The main change is to simplify the printing of numerical values to always use high precision, meaning print all significant digits of floating point values. Since our tests generate small integral values and the intent is generally to be testing mostly the exact arithmetic that happens on small integral values, in most cases this doesn't make any difference. But I found that RDNA3 float arithmetic produces non-exact results even on those values. As a result, I got values like 1+epsilon where 1 was expected, causing a test to fail (since we didn't know we needed to opt out from requiring exact results) and the test output cryptically printed both values as "1". The other change is to more consistently print the same number of rows and columns regardless of whether we are at the start or in the middle of a dimension, and to have that number be what we call "context" (before, it was "2 * context"). Also a seasonal emoji change. Signed-off-by: Benoit Jacob <[email protected]>
giacs-epic
pushed a commit
to giacs-epic/iree
that referenced
this pull request
Dec 4, 2024
A few things were needed: * Populate required fields in KnownTargets.cpp. * Support the case where the intrinsic vector operand size is greater than the load instruction size (here it is 16xf16 = 256 bit). * `buildMmaOperation` creates `vector.insert_strided_slice` to insert the new accumulator vectors into the accumulator tile. In doing so, it was relying on `vector.insert_strided_slice` implicit expand-shape semantics, in ways that worked for the shapes we had seen in CDNA3 but not here. Solved by explicitly expanding shapes with `vector.shape_cast` ops. * In thread-distribution code (populateOperandXxx), we needed to account for the nuance between two distinct thread grids: "layout" vs "distribution". In the case of RDNA3, there is a distribution-only dimension that isn't reflected in the layout-centric TileSwizzle's. * On RDNA3, some float arithmetic is strongly non-IEEE754-compliant: even exactly-representable small integral values, on which float arithmetic should be exact, have epsilon numerical errors! Addressed by tolerance. * Fix a bug: the doubly-nullable type `std::optional<IREE::GPU::DataTiledMMAAttr>` tricked us, change to `IREE::GPU::DataTiledMMAAttr`. --------- Signed-off-by: Benoit Jacob <[email protected]> Signed-off-by: Giacomo Serafini <[email protected]>
giacs-epic
pushed a commit
to giacs-epic/iree
that referenced
this pull request
Dec 4, 2024
Working on iree-org#18980 let me spend quality time with e2e matmul tests and suggested some changes. The main change is to simplify the printing of numerical values to always use high precision, meaning print all significant digits of floating point values. Since our tests generate small integral values and the intent is generally to be testing mostly the exact arithmetic that happens on small integral values, in most cases this doesn't make any difference. But I found that RDNA3 float arithmetic produces non-exact results even on those values. As a result, I got values like 1+epsilon where 1 was expected, causing a test to fail (since we didn't know we needed to opt out from requiring exact results) and the test output cryptically printed both values as "1". The other change is to more consistently print the same number of rows and columns regardless of whether we are at the start or in the middle of a dimension, and to have that number be what we call "context" (before, it was "2 * context"). Also a seasonal emoji change. Signed-off-by: Benoit Jacob <[email protected]> Signed-off-by: Giacomo Serafini <[email protected]>
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.
A few things were needed:
buildMmaOperationcreatesvector.insert_strided_sliceto insert the new accumulator vectors into the accumulator tile. In doing so, it was relying onvector.insert_strided_sliceimplicit expand-shape semantics, in ways that worked for the shapes we had seen in CDNA3 but not here. Solved by explicitly expanding shapes withvector.shape_castops.std::optional<IREE::GPU::DataTiledMMAAttr>tricked us, change toIREE::GPU::DataTiledMMAAttr.