Page MenuHomePhabricator

Introduce linalg.vecmat
ClosedPublic

Authored by burmako on Tue, Sep 8, 9:49 AM.

Details

Summary

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.

Diff Detail

Event Timeline

burmako created this revision.Tue, Sep 8, 9:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
burmako requested review of this revision.Tue, Sep 8, 9:49 AM
mravishankar requested changes to this revision.Tue, Sep 8, 10:21 AM
mravishankar added a subscriber: mravishankar.

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

  1. The ops to loops method now needs to be updated.
  2. 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.

This revision now requires changes to proceed.Tue, Sep 8, 10:21 AM

@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 ?

burmako updated this revision to Diff 290616.Tue, Sep 8, 5:12 PM

Fixed the definition of linalg.vecmat - the original definition was wrong.

@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.

mravishankar accepted this revision.Wed, Sep 9, 1:51 PM

@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 .

This revision is now accepted and ready to land.Wed, Sep 9, 1:51 PM

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.

nicolasvasilache accepted this revision.Thu, Sep 10, 7:56 AM

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.

burmako edited the summary of this revision. (Show Details)Thu, Sep 10, 8:34 AM
burmako updated this revision to Diff 290998.Thu, Sep 10, 9:17 AM

Rebase to account for recent changes

This revision was automatically updated to reflect the committed changes.