-
Notifications
You must be signed in to change notification settings - Fork 5
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
Broadcast support for elementwise ops #148
Conversation
…amic dimensions handling based on symbols.
} | ||
} | ||
|
||
auto squeezed = builder.create<tensor::CollapseShapeOp>(loc, inputs[i], squeeze_map); |
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.
Discussed this way of "preparation for broadcast" with @adam-smnk.
I think this approach is not convenient because the whole reason for using named ops is simplicity. If a widely used broadcast semantics requires tricks, it is not simple to use, and too low level to be an entry point dialect in OV case.
@adam-smnk, @rengolin, please propose a better way of doing that.
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.
Digging a bit deeper, it is the intended approach in line with the original op design:
https://discourse.llvm.org/t/rfc-primitive-ops-add-broadcastop-to-linalg/66313
Size-1 expansion is deliberately forbidden.
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 can use generics for now, but we're discussing adding broadcast and transpose to named ops.
https://discourse.llvm.org/t/rfc-transpose-attribute-for-linalg-matmul-operations/80092/
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.
I don't have experience using linalg generics in C++, so cannot quickly produce this code. I will merge it as-is with collapse/broadcast.
…n. Forced f32 in the conversion pipeline.
TODO:
Now the entire graph (except tensor constants) from #146 (comment) is converted as a single MLIR function: