-
Notifications
You must be signed in to change notification settings - Fork 625
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
Add a lowering of vector.interleave
to vector.shuffle
#17346
Comments
Is this an mlir issue or iree issue? To me it sound like this should go to vector-to-spirv and vector-to-llvm? |
I was thinking vector-to-vector, rewriting Then also a IREE-side change to insert that new pattern into codegen pipelines; and also IREE-side, remembering to drop the revert of llvm/llvm-project#89131 at the following LLVM integrate. |
Oh OK, so this pattern is already there, just need to add add it to iree pipelines. Makes sense. |
no no, a file is there but it only contains an unrelated |
On the spir-v side, I don't think there's any better lowering we could use anyway, spir-v has its own shuffle ops. |
…800a3 (#17330) * torch-mlir integrated at bce800a. * llvm-project integrated at 2083e97e plus local changes: * Reverted llvm/llvm-project#89131 locally: while this change is good in its own right, the `vector.interleave` that it generates (instead of `vector.shuffle`) are not handled by some GPU codegen lowerings. * Filed #17346. * Cherry-picked Bazel build fix: llvm/llvm-project#91654 * Several e2e tests have been temporarily disabled, follow-up work is needed to reenable them: #17344 --------- Co-authored-by: MaheshRavishankar <[email protected]> Co-authored-by: Scott Todd <[email protected]>
@kuhar @qedawkins , does this look like what we discussed? llvm/llvm-project#91800 iree/compiler/src/iree/compiler/Codegen/SPIRV/SPIRVInitialVectorLowering.cpp Lines 298 to 511 in a3b7e12
|
Otherwise here also seems fine:
|
Nice, that looks great to me. In terms of the IREE side, I think some combination of adding the
And then the interleave to shuffle can either go in the same place, or somewhere near here:
Basically after decomposing to 1d and before unrolling to 1/2/3/4 vector elements. |
Correct me if otherwise, but I was thinking it had to happen before unrolling to <= 4 elements? Unless interleave implements the unrolling interface already. |
My hope would be that unrolling could break it down to source vectors in some cases already, but I haven't checked if it support the unrolling interface. |
This is also a good option if it doesn't support unrolling. |
I have updated the PR with that, thanks for the tip!
@kuhar @qedawkins I am just trying to fix the immediate issue that is forcing us to carry a local LLVM revert here. This issue only seems to involve 1D vectors as far as I have seen so far. |
Makes sense, then I would say any point before here is likely to work:
|
So should the upstream PR do it in VectorToSPIRV.cpp#L812 or not? If it should do that upstream, then nothing needs to be done on the IREE side, right? |
I'm guessing we'll still need to do something on the IREE side because in addition to the requirement that all vectors must be |
but that sounds like solving a more general problem than my immediate concern of flattening our llvm integration. The upstream change that we have to carry a local revert of isn't changing the number of elements in a vector, or the rank of a vector. It's just changing vector.shuffle-on-1d-vector to vector.interleave-on-1d-vector. |
…le in VectorToSPIRV (#91800) Context: iree-org/iree#17346. Test IREE integrate showing it's fixing the problem it's intended to fix, i.e. it allows IREE to drop its local revert of #89131: iree-org/iree#17359 This is added to VectorToSPIRV because SPIRV doesn't currently handle `vector.interleave` (see motivating context above). This is limited to 1D, non-scalable vectors.
…le in VectorToSPIRV (#92012) This is the second attempt at merging #91800, which bounced due to a linker error apparently caused by an undeclared dependency. `MLIRVectorToSPIRV` needed to depend on `MLIRVectorTransforms`. In fact that was a preexisting issue already flagged by the tool in https://discourse.llvm.org/t/ninja-can-now-check-for-missing-cmake-dependencies-on-generated-files/74344. Context: iree-org/iree#17346. Test IREE integrate showing it's fixing the problem it's intended to fix, i.e. it allows IREE to drop its local revert of #89131: iree-org/iree#17359 This is added to VectorToSPIRV because SPIRV doesn't currently handle `vector.interleave` (see motivating context above). This is limited to 1D, non-scalable vectors.
…800a3 (iree-org#17330) * torch-mlir integrated at bce800a. * llvm-project integrated at 2083e97e plus local changes: * Reverted llvm/llvm-project#89131 locally: while this change is good in its own right, the `vector.interleave` that it generates (instead of `vector.shuffle`) are not handled by some GPU codegen lowerings. * Filed iree-org#17346. * Cherry-picked Bazel build fix: llvm/llvm-project#91654 * Several e2e tests have been temporarily disabled, follow-up work is needed to reenable them: iree-org#17344 --------- Co-authored-by: MaheshRavishankar <[email protected]> Co-authored-by: Scott Todd <[email protected]>
…800a3 (iree-org#17330) * torch-mlir integrated at bce800a. * llvm-project integrated at 2083e97e plus local changes: * Reverted llvm/llvm-project#89131 locally: while this change is good in its own right, the `vector.interleave` that it generates (instead of `vector.shuffle`) are not handled by some GPU codegen lowerings. * Filed iree-org#17346. * Cherry-picked Bazel build fix: llvm/llvm-project#91654 * Several e2e tests have been temporarily disabled, follow-up work is needed to reenable them: iree-org#17344 --------- Co-authored-by: MaheshRavishankar <[email protected]> Co-authored-by: Scott Todd <[email protected]> Signed-off-by: Lubo Litchev <[email protected]>
In llvm integrate #17330 we have to locally revert llvm/llvm-project#89131 because it causes
vector.interleave
to be created instead ofvector.shuffle
, and some GPU codegen backends expectedvector.shuffle
and are not handlingvector.interleave
.llvm/llvm-project#89131 is good in itself though, as
vector.interleave
is more constrained than generalvector.shuffle
. We just need a lowering pattern fromvector.interleave
tovector.shuffle
to be inserted into codegen pipelines. Then we will be able to drop the local revert of llvm/llvm-project#89131.FYI @KoolJBlack @qedawkins @kuhar
The text was updated successfully, but these errors were encountered: