Skip to content

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.

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