Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPU data tiling: Refine tile dimensions, more preparation for thread distribution. #18556

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

bjacob
Copy link
Contributor

@bjacob bjacob commented Sep 19, 2024

  • The unrolling factors in DataTiledMMAAttr get split between plain unrolling and unroll-to-subgroups.
  • The dimensions in TileSwizzle get an enum telling if they are cross-thread / cross-instruction.
  • getSwizzle gets moved to GPUTileSwizzleUtils as it is going to be used in codegen outside of MaterializeEncoding.

@bjacob bjacob marked this pull request as ready for review September 20, 2024 18:55
@bjacob bjacob requested a review from Max191 September 21, 2024 01:02
@bjacob
Copy link
Contributor Author

bjacob commented Sep 23, 2024

I had a change of heart and decided that an enum was better after all. The thing that changed since I had earlier favored separate booleans, is that regarding the unrolling dimensions, we are now splitting them into separate unroll-to-subgroups vs plain unroll dimensions. The former are cross-thread, the latter are cross-intrinsic. Thus, we never anymore have a dimensions that's simultaneously cross-thread and cross-intrinsic, so these things aren't orthogonal after all. The last commit added to this PR implements that change. @Max191 @qedawkins .

Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just few nits. I'll need to learn more about the intention of Kind enum class. Let's catch up on Wed.

Copy link
Contributor

@Max191 Max191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just leaving a reminder to update the comments as we discussed offline.

Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
@bjacob bjacob enabled auto-merge (squash) September 24, 2024 20:14
@bjacob bjacob merged commit 9158a90 into iree-org:main Sep 24, 2024
35 checks passed
Comment on lines +12 to +22
llvm::raw_ostream &operator<<(llvm::raw_ostream &os,
TileSwizzle::Dim::Kind kind) {
switch (kind) {
case TileSwizzle::Dim::Kind::Internal:
return os << "Internal";
case TileSwizzle::Dim::Kind::CrossThread:
return os << "CrossThread";
case TileSwizzle::Dim::Kind::CrossIntrinsic:
return os << "CrossIntrinsic";
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke the gcc build: https://github.com/iree-org/iree/actions/runs/11030265747/job/30634464951#step:4:6937

[6307/8097] Building CXX object compiler/src/iree/compiler/Codegen/Common/CMakeFiles/iree_compiler_Codegen_Common_Common.objects.dir/TileSwizzle.cpp.o
FAILED: compiler/src/iree/compiler/Codegen/Common/CMakeFiles/iree_compiler_Codegen_Common_Common.objects.dir/TileSwizzle.cpp.o 
/usr/local/bin/ccache /usr/bin/g++-9  -I/__w/iree/iree -I/__w/iree/iree/build-gcc -I/__w/iree/iree/third_party/llvm-project/llvm/include -I/__w/iree/iree/build-gcc/llvm-project/include -I/__w/iree/iree/third_party/llvm-project/mlir/include -I/__w/iree/iree/build-gcc/llvm-project/tools/mlir/include -I/__w/iree/iree/third_party/llvm-project/lld/include -I/__w/iree/iree/build-gcc/llvm-project/tools/lld/include -I/__w/iree/iree/compiler/src -I/__w/iree/iree/build-gcc/compiler/src -I/__w/iree/iree/runtime/src -I/__w/iree/iree/build-gcc/runtime/src -I/__w/iree/iree/build-gcc/runtime/src/iree/base/internal/flatcc -I/__w/iree/iree/llvm-external-projects/iree-dialects/include -I/__w/iree/iree/build-gcc/llvm-external-projects/mlir-iree-dialects/include -isystem /__w/iree/iree/third_party/flatcc/include -O3  -fPIC -fvisibility=hidden -fno-rtti -fno-exceptions -Wall -Werror -Wno-error=deprecated-declarations -Wno-address -Wno-address-of-packed-member -Wno-comment -Wno-format-zero-length -Wno-uninitialized -Wno-overloaded-virtual -Wno-invalid-offsetof -Wno-sign-compare -Wno-unused-function -Wno-unknown-pragmas -Wno-unused-but-set-variable -Wno-misleading-indentation -fmacro-prefix-map=/__w/iree/iree=iree -I/__w/iree/iree/third_party/flatcc/include/ -I/__w/iree/iree/third_party/flatcc/include/flatcc/reflection/ -std=gnu++17 -MD -MT compiler/src/iree/compiler/Codegen/Common/CMakeFiles/iree_compiler_Codegen_Common_Common.objects.dir/TileSwizzle.cpp.o -MF compiler/src/iree/compiler/Codegen/Common/CMakeFiles/iree_compiler_Codegen_Common_Common.objects.dir/TileSwizzle.cpp.o.d -o compiler/src/iree/compiler/Codegen/Common/CMakeFiles/iree_compiler_Codegen_Common_Common.objects.dir/TileSwizzle.cpp.o -c /__w/iree/iree/compiler/src/iree/compiler/Codegen/Common/TileSwizzle.cpp
/__w/iree/iree/compiler/src/iree/compiler/Codegen/Common/TileSwizzle.cpp: In function 'llvm::raw_ostream& mlir::iree_compiler::operator<<(llvm::raw_ostream&, mlir::iree_compiler::TileSwizzle::Dim::Kind)':
/__w/iree/iree/compiler/src/iree/compiler/Codegen/Common/TileSwizzle.cpp:22:1: error: control reaches end of non-void function [-Werror=return-type]
   22 | }
      | ^
cc1plus: all warnings being treated as errors

Could assert on the default / fallthrough case, or implement some logic in case an unhandled enum value gets passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging! I have added a fix to #18547 which I'm about to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants