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.

Sign up for GitHub

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

Jump to bottom

[DmaComposition] Rework logic to enable new composition opportunity #1042

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[DmaComposition] Rework logic to enable new composition opportunity #1042

wants to merge 2 commits into from

Conversation

newling
Copy link
Contributor

@newling newling commented Jan 20, 2025 •
edited
Loading

  • slight modification to logic used for compositing DMAs, makes use of global offset difference (please see comments in code)
  • more aggressive folding of unit dimensions
  • new unit test in combine_strided_ops.mlir shows the case that was being missed in an end-to-end matmul transpose b with the new pipeline pack-peel-4-level-tiling

TODO(newling) add more tests with non-constant offsets.

@newling newling force-pushed the dma_canonicalization branch 2 times, most recently from 7b21be2 to 72c43ce Compare January 20, 2025 22:42
@newling newling changed the title [WIP][DmaComposition] Simply and hit more cases [DmaComposition] Rework logic to enable new composition opportunity Jan 20, 2025
@newling newling force-pushed the dma_canonicalization branch from 72c43ce to b526685 Compare January 21, 2025 22:41
@newling newling changed the title [DmaComposition] Rework logic to enable new composition opportunity [WIP][DmaComposition] Rework logic to enable new composition opportunity Jan 21, 2025
@newling newling force-pushed the dma_canonicalization branch from 2709490 to 675c6a8 Compare January 22, 2025 17:50
@newling newling changed the title [WIP][DmaComposition] Rework logic to enable new composition opportunity [DmaComposition] Rework logic to enable new composition opportunity Jan 22, 2025
@newling newling marked this pull request as ready for review January 22, 2025 17:55
yzhang93
yzhang93 reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@yzhang93 yzhang93 left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor comments.

compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
jtuyls
jtuyls requested changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

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

This is a great refactor with additional combinations being enabled, thanks for the thorough logic and great explanations!

compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.h Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/test/AMDAIEDmaUtilsTest.cpp Outdated Show resolved Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/Utils/AMDAIEDmaUtils.cpp
/// by the access pattern B, can be merged into a single access pattern.
///
/// \return global_offset(X) - global_offset(Y).
std::optional<int64_t> getGlobalOffsetDifference(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be useful to add unit tests for this utility (and other similar utilities).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some in the .cpp test file for the constant cases. For the non-constant cases, I'm in the process of adding in the combine_strided_ops.mlir, need to add some negative cases though (non-constant global offset difference). Tomorrow..

@newling newling force-pushed the dma_canonicalization branch 2 times, most recently from 68f4973 to 55cab2a Compare January 24, 2025 16:08
@newling
Copy link
Contributor Author

newling commented Jan 24, 2025

This is a great refactor with additional combinations being enabled, thanks for the thorough logic and great explanations!

Thanks! I think I've addressed all the comments now, let me know if you think it should have more testing etc.

jtuyls
jtuyls reviewed Jan 24, 2025
View reviewed changes
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/test/AMDAIEDmaUtilsTest.cpp

namespace {

using namespace mlir;
using namespace mlir::iree_compiler::AMDAIE;

SmallVector<int64_t> fromOpFoldResults(SmallVector<OpFoldResult> ofrs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you use getConstantIntValues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops second time. Changed

jtuyls
jtuyls approved these changes Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit above.

@newling newling force-pushed the dma_canonicalization branch from 55cab2a to eb5a06f Compare January 24, 2025 21:33
@newling newling enabled auto-merge (squash) January 24, 2025 21:34
@newling newling force-pushed the dma_canonicalization branch from 7962217 to 504ca1a Compare January 25, 2025 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yzhang93 yzhang93 yzhang93 left review comments

@jtuyls jtuyls jtuyls approved these changes

@MaheshRavishankar MaheshRavishankar Awaiting requested review from MaheshRavishankar MaheshRavishankar is a code owner

@nirvedhmeshram nirvedhmeshram Awaiting requested review from nirvedhmeshram nirvedhmeshram is a code owner

@Abhishek-Varma Abhishek-Varma Awaiting requested review from Abhishek-Varma Abhishek-Varma is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newling @yzhang93 @jtuyls