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

[LowerToAIE] Handle linalg ops like other ops in switch statement #693

Closed
wants to merge 1 commit into from

Conversation

newling
Copy link
Contributor

@newling newling commented Aug 21, 2024

This is (now) just a code simplification. Linalg Ops don't need special handling.

@@ -396,9 +384,6 @@ LogicalResult coreToAIE(IRRewriter &rewriter, AMDAIE::CoreOp coreOp,
return coreReleaseOpToAIE(rewriter, releaseOp, mapper,
toBeErased);
})
.Case<linalg::LinalgOp>([&](auto linalgOp) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just reading the description and PR, this seems like it is dropping general handling of LinalgOp. Is that to mean that we dont expect LinalgOps to survive till here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of handling them as a special case:

They are now handled like other not-special ops:

We (I) do expect LinalgOps to survive til here.

@newling
Copy link
Contributor Author

newling commented Aug 22, 2024

I thought this was a fix for the non-deterministic crash I'm seeing in this pass with convolution, but it's not. I think I've found the actual problem (IRMapping not being updated when operations are erased) I'll post a fix for this tomorrow

@makslevental
Copy link
Collaborator

I thought this was a fix for the non-deterministic crash I'm seeing in this pass with convolution, but it's not. I think I've found the actual problem (IRMapping not being updated when operations are erased) I'll post a fix for this tomorrow

really? where?

@newling
Copy link
Contributor Author

newling commented Aug 22, 2024

really? where?

#694

@newling newling changed the title [LowerToAIE] Change handling of linalg operations (fix for conv lowering) [LowerToAIE] Handle LinalgOps like other ops in switch statement Aug 22, 2024
@newling newling changed the title [LowerToAIE] Handle LinalgOps like other ops in switch statement [LowerToAIE] Handle linalg ops like other ops in switch statement Aug 22, 2024
@newling
Copy link
Contributor Author

newling commented Aug 26, 2024

Bundled with #705

@newling newling closed this Aug 26, 2024
@newling newling deleted the linalg_demining branch August 29, 2024 18:26
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.

3 participants