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

Move AMDAIEAssignChannelsPass before AMDAIEAssignNpuDmaBdIdsPass #980

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

Yu-Zhewen
Copy link
Contributor

This PR is to resolve the following comment:

// TODO(jornt): Temporarily use channel 0 for all DMAs. This should
// return correct results for Shim channels, however, for generality
// towards other DMAs and future hardware generations, channel
// assignment should happen before BD assignemnt. This requires more
// refactoring.
const uint32_t channel = 0;

  • passes are now reordered
  • add a utility function to retrieve ChannelOp from given NpuDmaCpyNdOp
  • unit test assign-npu-dma-bd-ids.mlir is refactored

@@ -61,6 +61,32 @@ FailureOr<AMDAIE::TileOp> getGeneratorTileOp(
return tileOp;
};

/// Utility to retrieve a ChannelOp from a DMA copy operation.
template <CopyOpOperateOn OperateOn>
FailureOr<AMDAIE::ChannelOp> getChannelOp(AMDAIE::NpuDmaCpyNdOp &npuDmaOp) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make this reusable, could you make this a method of NpuDmaCpyNdOp instead? You likely need to expose it as getSourceChannelOp and getTargetChannelOp and use this function under the hood.

Comment on lines 185 to 186
AMDAIE::ChannelOp channelOp = maybeChannelOp.value();
uint32_t channel = channelOp.getValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
AMDAIE::ChannelOp channelOp = maybeChannelOp.value();
uint32_t channel = channelOp.getValue();
uint32_t channel = maybeChannelOp.value().getValue();

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

@Yu-Zhewen Yu-Zhewen enabled auto-merge (squash) December 11, 2024 11:21
@Yu-Zhewen Yu-Zhewen merged commit 71e17ed into main Dec 11, 2024
8 checks passed
@Yu-Zhewen Yu-Zhewen deleted the zhewen_moveAssignChannels branch December 11, 2024 11:54
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