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

[DT] Unify encoding materialization pass into a single pass. #19454

Merged
merged 9 commits into from
Dec 16, 2024

Conversation

hanhanW
Copy link
Contributor

@hanhanW hanhanW commented Dec 11, 2024

The revision creates a generic materialization pass and uses it for backends that implement data-tiling. After months of development, we identify that the needs of GPU is a superset of the needs of CPU. To be more specific, it has the additional "swizzle" field in terms of layout. It means that the GPU set_encoding/unset_encoding lowering patterns cover the needs of CPU path. The lowering of contraction ops is different. CPU lowers it to mmt4d ops, while GPU lowers it to multi_mma op. However, the lowering of contraction is implemented through attribute interface. Thus, we can have a generic pattern to lower contraction ops.

To make the review process much easier, the revision is created by 5 commits.

  1. It directly creates the MaterializeEncoding pass and copy-paste the GPU patterns: SetEncodingOpLoweringConversion, UnSetEncodingOpLoweringConversion, and MaterializeContractionOp. In the first commit, it also updates the GPU tests to use the new pass.
  2. The GPU data-tiling does not support element-wise generic op lowering atm. The second commit moves the pattern to shared pattern set and bail out when swizzle is present. This is an NFC for both pipelines.
  3. The third commit replaces the existing materialization pass with the generic pass, and deletes all the legacy passes.
  4. The four commit moves the lit tests from Common/[CPU|GPU]/test to Common/test.
  5. Now there are duplicate patterns for set_encoding, unset_encoding, and contraction ops lowering. The last commit deletes the legacy patterns, and move the patterns from MaterializeEncoding.cpp to where the legacy patterns locate. Furthermore, it renames the file as MaterializeEncodingPatterns.cpp.

The revision retains the MaterializeEncodingIntoNop pass, and add a TODO item. Because it is still used by MaterializeHomogeneousEncoding pass. It can be deleted once we deprecate the early materialization path.

@hanhanW hanhanW changed the base branch from main to users/hanhanW/data-tiling-cleanups-narrow-n December 11, 2024 11:34
@hanhanW
Copy link
Contributor Author

hanhanW commented Dec 11, 2024

This depends on #19452 and #19453. I push the parent commits to an upstream branch, so it is ready for review.

@hanhanW hanhanW marked this pull request as ready for review December 11, 2024 12:44
@hanhanW hanhanW requested review from bjacob and Max191 and removed request for benvanik, antiagainst and qedawkins December 11, 2024 12:44
Copy link
Contributor

@bjacob bjacob left a comment

Choose a reason for hiding this comment

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

Very nice reorganization!

@@ -161,6 +161,11 @@ const char *getIreeArchNameForTargetTriple(llvm::Triple triple) {
return "unknown";
}

bool isLLVMCPUBackend(IREE::HAL::ExecutableTargetAttr targetAttr) {
return targetAttr &&
targetAttr.getBackend().getValue().starts_with("llvm-cpu");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just check equality ? Why starts_with? If that was copying the vmvx case below, I might have done that out of ignorance in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. Thanks for pointing it out! I'm mainly following the other cases, but can definitely check equality.

Signed-off-by: hanhanW <hanhan0912@gmail.com>
also update cpu targets in llvmcpu_materialize_encoding.mlir

Signed-off-by: hanhanW <hanhan0912@gmail.com>
Signed-off-by: hanhanW <hanhan0912@gmail.com>
Signed-off-by: hanhanW <hanhan0912@gmail.com>
…rns.cpp

Signed-off-by: hanhanW <hanhan0912@gmail.com>
Signed-off-by: hanhanW <hanhan0912@gmail.com>
@hanhanW hanhanW force-pushed the rework-materialize-encoding branch from 9005f5a to 0f6ac93 Compare December 16, 2024 06:08
@hanhanW hanhanW changed the base branch from users/hanhanW/data-tiling-cleanups-narrow-n to main December 16, 2024 06:08
@hanhanW hanhanW removed the request for review from ScottTodd December 16, 2024 06:09
Signed-off-by: hanhanW <hanhan0912@gmail.com>
@hanhanW hanhanW enabled auto-merge (squash) December 16, 2024 06:47
Signed-off-by: hanhanW <hanhan0912@gmail.com>
Signed-off-by: hanhanW <hanhan0912@gmail.com>
@hanhanW hanhanW force-pushed the rework-materialize-encoding branch from 1a13342 to 5224968 Compare December 16, 2024 09:51
@hanhanW hanhanW merged commit 05ce39f into iree-org:main Dec 16, 2024
38 checks passed
@hanhanW hanhanW deleted the rework-materialize-encoding branch December 16, 2024 10:12
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.

2 participants