[Codegen] Migrate PadOp/PackOp/UnPackOp to VectorizableOpInterface#23712
[Codegen] Migrate PadOp/PackOp/UnPackOp to VectorizableOpInterface#23712
Conversation
hanhanW
requested review from
MaheshRavishankar,
Max191 and
qedawkins
as code owners
|
|
||
| /// External model for tensor::PadOp. Different dialect, goes through | ||
| /// linalg::vectorize. | ||
| struct PadOpVectorizationModel |
There was a problem hiding this comment.
The two external models look almost identical to me. Can't we use the templated version from above for PadOp as well?
There was a problem hiding this comment.
It is intended because they are from different dialect. IMHO, the some upstream linalg ops are not completed because they don't implement LinalgOp interface. There may be extentions for such ops, while tensor.pad is a very special case in vectorization.
There was a problem hiding this comment.
But the dialect isn't part of the template, right? IMHO, we shouldn't duplicate code (DRY). If there is an extension of the upstream linalg ops in the future, we can always make a change here.
There was a problem hiding this comment.
I think you're over-abstracting the things. The duplication is okay to me if they are small. I've seen so many times about split -> merge -> split ->etc, and my thought is that if we already know it, we should do the split. It is clear to me that they have very different development attention, and the pack/unpack ops evolve much faster than pad op. Thus, splitting them in the first place is reasonable to me.
We also have good reasoning here, but not just "different dialect". As I mentioned, they should implement LinalgOp interface IMO. The other different thing is that tensor.pad op does not implement DPS interface, which can fall into different category easily. It is not happening now, but I'd like to make the isolation in the first place as I can see many difference between them.
There was a problem hiding this comment.
Ok, if you expect a split is likely to happen because of the evolution of pack/unpack, we can keep it as is.
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
from
37cef0e to
5aa5b62
Compare
hanhanW
requested review from
IanWood1,
krzysz00,
kuhar and
nirvedhmeshram
as code owners
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c3a
branch
from
ba02055 to
dcccae8
Compare
|
|
||
| /// External model for tensor::PadOp. Different dialect, goes through | ||
| /// linalg::vectorize. | ||
| struct PadOpVectorizationModel |
There was a problem hiding this comment.
Ok, if you expect a split is likely to happen because of the evolution of pack/unpack, we can keep it as is.
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
from
5aa5b62 to
a84722f
Compare
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c3a
branch
from
dcccae8 to
c8f23d0
Compare
Add external models for upstream linalg/tensor ops: - NonLinalgStructuredOpVectorizationModel for PackOp, UnPackOp - PadOpVectorizationModel for tensor::PadOp Signed-off-by: hanhanW <[email protected]>
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c3a
branch
from
c8f23d0 to
54492ef
Compare
3 participants
No new tests because it is an NFC in terms of functionality. It just follows different mechanism for vectorization.
It is a step towards https://lists.lfaidata.foundation/g/iree-technical-discussion/message/15
Assisted-by: Claude