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

[AMDAIEInsertIntoCores] minor simplification #1118

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

newling
Copy link
Contributor

@newling newling commented Feb 19, 2025

The arith ops are nested inside linalg ops, so they'll get hoisted into cores without explicitly listing them here

@newling newling marked this pull request as ready for review February 19, 2025 23:59
memref::ExtractStridedMetadataOp, func::CallOp, arith::ExtFOp,
arith::TruncFOp, arith::TruncIOp, vector::TransferReadOp,
vector::TransferWriteOp>(op);
memref::ExtractStridedMetadataOp, func::CallOp,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily, see matmul + truncf:

// input ${M}x${K}x${TYPE1}
// input ${K}x${N}x${TYPE1}

func.func @matmul_truncf(%arg0: tensor<${M}x${K}x${TYPE1}>, %arg1: tensor<${K}x${N}x${TYPE1}>) -> tensor<${M}x${N}x${TYPE1}>
{
  %cst = arith.constant ${ZERO} : ${TYPE2}
  %0 = tensor.empty() : tensor<${M}x${N}x${TYPE2}>
  %1 = linalg.fill ins(%cst : ${TYPE2}) outs(%0 : tensor<${M}x${N}x${TYPE2}>) -> tensor<${M}x${N}x${TYPE2}>
  %2 = linalg.matmul ins(%arg0, %arg1 : tensor<${M}x${K}x${TYPE1}>, tensor<${K}x${N}x${TYPE1}>)
    outs(%1: tensor<${M}x${N}x${TYPE2}>) -> tensor<${M}x${N}x${TYPE2}>
  %3 = arith.truncf %2 : tensor<${M}x${N}x${TYPE2}> to tensor<${M}x${N}x${TYPE1}>
  return %3: tensor<${M}x${N}x${TYPE1}>
}

Not sure how this PR is passing though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In one of the very early passes (convert-elementwise-to-linalg) It gets converted into a linalg.generic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks, that's great! Then we can assume all computational ops will be linalg ops, which makes these checks a lot less fragile and improves maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

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

@newling newling enabled auto-merge (squash) February 20, 2025 16:19
@newling newling merged commit d5baa2c into nod-ai:main Feb 20, 2025
7 checks passed
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