-
Notifications
You must be signed in to change notification settings - Fork 608
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
[Flow] Raise special linalg.generic
ops to linalg.fill
ops
#14773
Conversation
834441a
to
9211662
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.
Nice! Just one minor comment.
Value yieldOperand; | ||
for (Operation &bodyOp : linalgOp.getBlock()->getOperations()) { | ||
if (isa<linalg::YieldOp>(bodyOp)) | ||
yieldOperand = bodyOp.getOperand(0); |
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.
You also need to check that the yield operand is not the basic block argument of the linalg operation itself.
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.
Should be good now. Lmk what you think.
a02ba09
to
2281e94
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.
Sorry, forgot to push my comments from yesterday. I think this works. Left a few comments. There could be a cleanup of the logic that could be done after the fact to simplify the code. Happy to chat more.
// Check that the op body is only a linalg.yield op. | ||
Value yieldOperand; | ||
for (Operation &bodyOp : linalgOp.getBlock()->getOperations()) { | ||
if (isa<linalg::YieldOp>(bodyOp)) |
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.
Nit: please add {
around all statements (this is divergence in IREE style w.r.t LLVM style)
// Check that the operand of the linalg.yield op is not an argument of the | ||
// linalg.generic basic block | ||
for (Value blockArg : linalgOp.getBlock()->getArguments()) | ||
if (yieldOperand == blockArg) |
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.
Same nit here. Please add braces always in IREE.
c720f45
to
a488f29
Compare
rebased on top of main |
a488f29
to
892148d
Compare
No description provided.