[RVV] Add SpaceMiT IME narrow M0 mmt4d tiles - #24802
Conversation
Register 2x4/1x4 xsmtvdot truncations of the primary 3x4 grid so narrow-M workloads stay on IME. Fixes iree-org#24769. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Terence <terrencemahlatini@gmail.com>
egebeysel
left a comment
egebeysel
left a comment
There was a problem hiding this comment.
The ukernel LGTM, a few nits here and there. My biggest concern is the feature flag declobbering mechanism in the PR, I think we can merge the ukernel-only commit, and defer the declobbering to a separate PR with its own issue 😄 I can also use some help from you if you created that issue and described what the exact problem is, so that it's easier to wrap my head round in isolation. Thanks!
| // rows) share the same N0/K0 and only shrink M0, to avoid falling back to | ||
| // slower non-IME paths for narrow-M matmuls. Other VLENs / scalable mode |
There was a problem hiding this comment.
| // rows) share the same N0/K0 and only shrink M0, to avoid falling back to | |
| // slower non-IME paths for narrow-M matmuls. Other VLENs / scalable mode | |
| // rows) share the same N0/K0 and only shrink M0, to avoid excessive | |
| // padding for narrow-M matmuls. Other VLENs / scalable mode |
| rhs += IME_NT * panel_stride; | ||
|
|
||
| __asm__ volatile( | ||
| " vsetvli x0, %[vl], e8, m1, ta, ma \n\t" |
There was a problem hiding this comment.
does the vsetvli have to be here? do you even need to specify this explicitly? The same concern applies for all grid sizes, I missed it in my review the other time. I would expect the backend to insert this instruction where it's needed, ideally outside of the above K-loop.
Can you take a look at the assembly and see where this is happening, and if you explicitly need this here? Doing this inside the hot-K-loop could be quite slow if LLVM does not hoist it out.
There was a problem hiding this comment.
your assumption was right. looking at the assembly, vsetvli is already being inserted by the backend for the vle8 intrinsics right before the asm block and it's setting the same vtype (e8,m1) that vmadot needs. Which makes the explicit one inside the asm redudant and not something the backend would hoist out. I have dropped the instruction.
There was a problem hiding this comment.
just to be clear, is the vsetvli instruction inside the K-loop or not? Also, I'm guessing it also has a counter-part. Is that inside the instruction or not?
I've never measured the performance impact of this, but ideally, we would have vsetvli once at the beginning of the ukernel before zeroing/loading the ACC and one after storing to the ACC since probably everything uses the same vtype anyways.
Am I mising something? You don't have to do this in this PR, but it might be worth looking into and maybe placing the vsetvli's explicitly to avoid them being inside hot-K loops.
There was a problem hiding this comment.
yes there is a vsetvli zero, <avl>, e8, m1 inside the K-loop for the vle8s. After dropping the explicit instruction, this is now auto inserted by the backend right before the vle8 intrinsics. smt.vmadot does not emit its own, it just uses whatever type is live. The counterpart is the e32, m2 vsetvlis for the accumulators and those are outside the K-loop, one before the load and one after store. And the ukernel is not using the same vtype throughout. the ACC is e32, m2 / AVL=atom² (16) and the operand loads and smt.vmadot are e8, m1 / AVL=atom*K0 (32). IME picks the MAC atom from vl·SEW, so those have to differ.
I can look into the vtype transition costs and report on performance in a separate follow up.
There was a problem hiding this comment.
As we're currently not able to test IME in the CI anyways, fixing this test configuration problem should IMO not be a drive-by-fix of this ukernel PR. Could you maybe revert the second commit, open up an issue and describe the precise problem there? Then you can reuse that commit and we can take a look at that separately :) Thanks!
There was a problem hiding this comment.
Sure, will drop it and open a separate issue with the problem description.
a468190 to
bbc1865
Compare
| rhs += IME_NT * panel_stride; | ||
|
|
||
| __asm__ volatile( | ||
| " vsetvli x0, %[vl], e8, m1, ta, ma \n\t" |
There was a problem hiding this comment.
just to be clear, is the vsetvli instruction inside the K-loop or not? Also, I'm guessing it also has a counter-part. Is that inside the instruction or not?
I've never measured the performance impact of this, but ideally, we would have vsetvli once at the beginning of the ukernel before zeroing/loading the ACC and one after storing to the ACC since probably everything uses the same vtype anyways.
Am I mising something? You don't have to do this in this PR, but it might be worth looking into and maybe placing the vsetvli's explicitly to avoid them being inside hot-K loops.
Signed-off-by: Terence <terrencemahlatini@gmail.com>
Signed-off-by: Terence <terrencemahlatini@gmail.com>
bbc1865 to
88cfb11
Compare
|
@egebeysel fixed the DCO and you can find the declobbering issue here: #24859 |
2 participants
Summary
xsmtvdot) narrow mmt4d tiles (2×4 / 1×4 atom grids) for VLEN 256/1024/4096. only validated VLEN=256 on hardware.-Fixes #24769
Assisted by: Cursor cursoragent@cursor.com
Signed-off-by: Terence terrencemahlatini@gmail.com