-
Notifications
You must be signed in to change notification settings - Fork 658
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
[Codegen][Common] Add a pass to linearize memrefs #19332
base: main
Are you sure you want to change the base?
[Codegen][Common] Add a pass to linearize memrefs #19332
Conversation
0fb0475
to
85bdf08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Abhishek! Left a few comments.
Hi @MaheshRavishankar @qedawkins - I've addressed the comments. This is ready for re-reviewing. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor note
c9dc955
to
20747ec
Compare
Hi @krzysz00 @qedawkins @MaheshRavishankar - I've addressed all comments and the CI seems to be happy now - you may re-review/approve. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar pass came up in a recent discussion, would be good to get a review from @krzysz00 too
} | ||
|
||
static FailureOr<SmallVector<OpFoldResult>> | ||
getMixedOrigSize(Location loc, PatternRewriter &rewriter, Value sourceValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method could be simpler if you use memref::getMixedSizes
. Something like
MemRefType sourceType = llvm::cast<MemRefType>(sourceValue.getType());
Operation *sourceOp = sourceValue.getDefiningOp();
if (!isa<memref::AllocOp, memref::AllocaOp>(sourceOp) && !sourceType.hasStaticShape()) {
return failure();
}
return memref::getMixedSizes(rewriter, sourceValue);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar reasoning as previous thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quinn is right. We should be able to just use getMixedSizes
. I think you dont even need the if
condition that he has there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I've responded here.
To link what I think we might want to do, #19851 |
eb1957f
to
a37f8a4
Compare
-- This commit creates a pass to linearize memrefs. -- The pass `iree-linearize-memrefs` will be iteratively worked upon to make it an inter-procedural pass. -- Currently it supports limited operations. Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
Signed-off-by: Abhishek Varma <[email protected]>
a37f8a4
to
67a3439
Compare
-- This commit creates a pass to linearize memrefs.
-- The pass
iree-linearize-memrefs
will be iteratively worked upon to make it an inter-procedural pass.-- Currently it supports limited operations.
Signed-off-by: Abhishek Varma [email protected]