This patch adds a new named structured op to accompany linalg.matmul and
linalg.matvec. We needed it for our codegen, so I figured it would be useful
to add it to Linalg.
Details
- Reviewers
nicolasvasilache aartbik mravishankar - Commits
- rG5638df195048: Introduce linalg.vecmat
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Not specific to the patch itself, but I am really concerned about the ballooning surface area of named ops in Linalg. I mentioned it before with the autogeneration from TC language approach to begin with, that one of the great things about Linalg is that many of the ops from dialects like MHLO (which have more justification for wide surface area of ops) can be boiled down to a "few" ops in Linalg. Adding more such operations here seems to be going away from that really useful feature. For example in this patch itself
- The ops to loops method now needs to be updated.
- The vectorization patterns now needs to be updated.
This is only going to get worse, and writing transformations is going to be really painful. For example in IREE, we have been working with linalg.conv. Now there are 7 versions of convolution in Linalg. We can use interfaces to solve a lot of this problem, but looks like the transformations above did not use interfaces. Either there is a fundamental reason for that, or its not implemented. If its the latter, that should probably addressed sooner rather than later.
Specifically for this patch, the op definition seems to just be linalg.matvec with operands swapped. Not sure I understand why this necessitates a separate operation.
@burmako ah my apologies, I thought you wanted a linalg op that would operate on memref<vector>.
Vecmat indeed looks just like matvec with permuted operands, as @mravishankar points out.
It seems your uses of vecmat could easily be turned into matvec ?
@mravishankar @nicolasvasilache Sorry for the confusion! My original patch messed up the definition of vecmat, making it look like it was just matvec with the parameters swapped. Please take a look at the new version of the patch.
I understand the concerns with increasing the surface area of named ops in Linalg, and I understand that this alone might be ground for rejecting this patch. Please let me know what you think about the new formulation though.
@burmako This makes sense. I suspected this is what you were planning for, but as I said, it is not specific to this patch. I understand why you need this.
FWIW
vecmat(y, A) = transpose(matvec(transpose(A), transpose(y)))
So we can always fold these away into a smaller op surface area, but this is effectively what the MHLO -> Linalg conversion does.
As I said earlier, not related to this patch, but it would be good to have some guidance/documentation about when a new named op is needed.
Accepting from my side. Please wait for @nicolasvasilache .
I chatted with Nicolas about this earlier today, and he was okay with this idea.
Please drop these from the commit summary - you could include it in a separate comment below.
I am not concerned. by having many ops as long as the underlying mechanisms are shared.
Of course, I agree that more core work is needed to get new op definitions to be just the 4 lines in .tc.
This is just a bit of tablegen + macro away so I am not worried that a future CL will make things more automatic.