-
Notifications
You must be signed in to change notification settings - Fork 30
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
[Tiling] Packing for convolution #756
Comments
I think the main concern is whether the generated date layout can be vectorized. And the linalg.generic's operands seem to be problematic to me. |
It looks ok to me, can you explain your concern? I think it's just a matmul with m=n=4 k=8 on contiguous slices of L1 allocations. |
I didn't see the indexing maps for the linalg.generic operands, so not sure what has been done. Is there an implicit collapse of dimension for |
Oops, here are the maps:
As an aside, note that Now let's look more closely at the linalg.generic (I'm going to try and convince you that it's just a matmul...). Copying the inner loop from before in a more readable way:
What are loop counts for each of the dimension
What's interesting here is that
|
So I think the one remaining problem for vectorization is to get the compiler to canonicalize this linalg.generic and then recognise that it's just a matmul. |
Update on this: I have a WIP pass which eliminates the singleton dimensions so that after the pass the linalg.generic is clearly a matmul, but vectorization now introduces a broadcast before the vector.contract. Investigating... |
I think I understand what is happening here. Can you post the method you are using to drop the unit-dimensions. There is an upstream method that allows you to drop unit dimensions and also control what dimensions are being dropped. If you are using this op
dropping the inner unit-dimension of |
Or if you post the IR post vectorization, that will give some clues |
I think previously it had compilation issue when lowered to vector.broadcast, but it's good to check if aievec can handle vector.broadcast now. |
Just noticed your comment @MaheshRavishankar and The pass I've written is basically the same as Removing all unit dimensions is exactly what I want. It isn't enough to just remove the reduction dimensions, because then the broadcasts doesn't get eliminated (vector.contract verifies that all dimensions to appear in either LHS or RHS). |
Use packing between L2 and L1 for convolution.
Using upstream MLIR packing I get the following.
So I currently think we don't need any modification to upstream MLIR.
I'll post a PR with my packing config later. It currently results in other issues cropping up (in air: out of tile memory. in objectFifo: some other crash).
The text was updated successfully, but these errors were encountered: