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

[ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination #1111

Merged
merged 5 commits into from
Feb 16, 2025

Conversation

Yu-Zhewen
Copy link
Contributor

@Yu-Zhewen Yu-Zhewen commented Feb 14, 2025

Following the discussion in #1063, connection ops generated for the control overlay do not initially have DMA users, as these are only added later when the content of control packets is determined. However, since connection ops are marked as pure in the IR, they can be wrongly eliminated by CSE and Canonicalization if they have no users at that stage.

@Yu-Zhewen Yu-Zhewen changed the title [ControlOverlay] Introduce dma_placeholder to preserve connection operation from Dead-Code Elimination [ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination Feb 14, 2025
@Yu-Zhewen Yu-Zhewen changed the title [ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination [WIP][ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination Feb 14, 2025
@Yu-Zhewen Yu-Zhewen changed the title [WIP][ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination [ControlOverlay] Introduce dma_placeholder to preserve connection ops from Dead-Code Elimination Feb 15, 2025
@Yu-Zhewen Yu-Zhewen marked this pull request as ready for review February 15, 2025 19:36
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.

Nice!

Comment on lines 607 to 609
if (failed(isCtrlFlow))
return connectionOp.emitOpError()
<< "could not determine if flow is control";
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
if (failed(isCtrlFlow))
return connectionOp.emitOpError()
<< "could not determine if flow is control";
if (failed(isCtrlFlow)) {
return connectionOp.emitOpError()
<< "could not determine if flow is control";
}

Comment on lines 592 to 594
This operation acts as a placeholder user for those `amdaie.connection` operations
that represent control flows, to prevent them from being dead-code eliminated.
This operation does not have any side effects on control code size.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would write this a bit more generally as there could be potential other uses for this placeholder in the future.

Suggested change
This operation acts as a placeholder user for those `amdaie.connection` operations
that represent control flows, to prevent them from being dead-code eliminated.
This operation does not have any side effects on control code size.
This operation acts as a placeholder user for `amdaie.connection` operations to prevent
them from being dead-code eliminated. This is used for control flow connections that are
inserted before control packets are generated because they need to be taken into account
together with data connections for routing. This operation does not have any side effects
on control code size.

@Yu-Zhewen Yu-Zhewen enabled auto-merge (squash) February 16, 2025 10:56
@Yu-Zhewen Yu-Zhewen merged commit fd4db47 into main Feb 16, 2025
6 of 7 checks passed
@Yu-Zhewen Yu-Zhewen deleted the zhewen_dma_placeholder branch February 16, 2025 11:46
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