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

Unify kernels for basic and nested affine for loop #217

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

Conversation

EthanMeng324
Copy link

Create a MLIR level unify functionality for two kernels. Merge the same affine for loop and use conditional branch to support different logic.

Copy link
Member

@chhzh123 chhzh123 left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! This PR with kernel analysis is very high quality! One suggestion: could you provide some MLIR test cases under the test folder? You can generate the IR from Allo and copy them as test cases here.

lib/Transforms/UnifyKernels.cpp Outdated Show resolved Hide resolved
namespace mlir {
namespace hcl {

bool compareAffineExprs(AffineExpr lhsExpr, AffineExpr rhsExpr) {
Copy link
Member

Choose a reason for hiding this comment

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

One question, did you do canonicalization somewhere? If I pass in d0+1 and 1+d0, will this function return true?

Copy link
Author

Choose a reason for hiding this comment

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

One question, did you do canonicalization somewhere? If I pass in d0+1 and 1+d0, will this function return true?

Yeah, I think that would be an issue for now. As I commented "Todo" in compareAffineMaps function, this is now a temporary solution. I was thinking about use a evaluation strategy to match some more sophisticated case as you mentioned. I will test and see how it work in future pr.

Comment on lines +160 to +161
static MlirModule UnifyKernels(MlirModule &mlir_mod1, MlirModule &mlir_mod2,
MlirContext &mlir_context) {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit weird that we pass in MLIRContext as the third argument. Can we make it a return module? Otherwise, providing some comments here describing what this mlir_context argument means is necessary.

Copy link
Author

Choose a reason for hiding this comment

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

It's a bit weird that we pass in MLIRContext as the third argument. Can we make it a return module? Otherwise, providing some comments here describing what this mlir_context argument means is necessary.

For now, I'm thinking in the frontend, we will create a context and use it to create module1 and module2. Then, we called this primitive. This third argument is definitely removable, because we can just extract the context of module1, which will be the same context. We can modify this interface later according to our design choice.

Copy link
Member

Choose a reason for hiding this comment

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

ok, sounds good

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