This patch removes the aarch64 instrinsic svget/svset/svcreate from llvm.
It also implements the InstCombine for vector.extract that used to be in svget.
Depends on: D131547
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
|---|---|---|
| 2417 ↗ | (On Diff #451374) | DstTy and VecTy are misnomers, because these are only non-nullptr for FixedWidthVectors, whereas the code you added works for both ScalableVectors and FixedWidthVectors. if (auto *FVTy = dyn_cast<FixedVectorType>(Vec->getType())) {
if (auto *DstFVTy = dyn_cast<FixedVectorType>(II->getType())) {
...
}
} |
| 2437 ↗ | (On Diff #451374) | Should this combine happen before the combine above? |
| llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-tuple-extract.ll | ||
| 1 ↗ | (On Diff #451374) | Does this test really need to be AArch64/SVE specific? Or can it live in llvm/test/Transforms/InstCombine ? |
| llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
|---|---|---|
| 2437 ↗ | (On Diff #451374) | I was not sure if I should change the order between fixed and generic. But now it is changed. |
Comment Actions
Can you also add a test for fixed-width vectors?
| llvm/test/Transforms/InstCombine/sve-intrinsic-opts-tuple-extract.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #452647) | Please also remove sve- from the name and any references to aarch64/SVE, e.g.
|
Comment Actions
@sdesmalen I addressed your comment and made the instcombine test for the vector.extract generic.
There is no dependency on aarch64.
I've used vector.insert to return a wider vector instead of using st4.
Is that fine?
| llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
|---|---|---|
| 2423 ↗ | (On Diff #452978) | InsertValue ? |
| llvm/test/Transforms/InstCombine/opts-tuples-extract-intrinsic.ll | ||
| 18–33 ↗ | (On Diff #452978) | I find this test largely incomprehensible :) Maybe you can split out the two cases in the code into two separate tests:
For (1): define <vscale x 16 x i8> @test_extract_insert_same_idx(<vscale x 64 x i8> %v0, <vscale x 16 x i8> %v1) {
; CHECK-LABEL: @test_extract_insert_same_idx(
; CHECK-NEXT: ret <vscale x 16 x i8> %v1
%vec.ins = call <vscale x 64 x i8> @llvm.vector.insert.nxv64i8.nxv16i8(<vscale x 64 x i8> %v0, <vscale x 16 x i8> %v1, i64 48)
%vec.ext = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %vec.ins, i64 48)
ret <vscale x 16 x i8> %vec.ext
} |
Comment Actions
Rewrite Instcombine test for vector extract. Now we have a test for when:
The indexes for extract and insert are equal
The indexes are different
The indexes are equal but the return and insert vector type are different
| llvm/test/Transforms/InstCombine/opts-tuples-extract-intrinsic.ll | ||
|---|---|---|
| 18–33 ↗ | (On Diff #452978) | I have changed as you said. |
Comment Actions
Could you move the code that adds the combine for llvm.vector.extract/insert to a separate patch (along with the beautiful new test), so that we can land that patch before landing D131547?
That also makes this patch simpler, as it will just remove all the svget/svset/svcreate stuff without any other changes.
| llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
|---|---|---|
| 2441 ↗ | (On Diff #453557) | nit: if you write the code as: auto *FVTy = dyn_cast<FixedVectorType>(Vec->getType());
auto *DstFVTy = dyn_cast<FixedVectorType>(ReturnType);
if (FVTy && DstFVTy) {
...
}you can avoid indenting the whole block (which reduces the size of the diff) |
| llvm/test/Transforms/InstCombine/opts-tuples-extract-intrinsic.ll | ||
| 18–33 ↗ | (On Diff #452978) | That looks great, thank you! |
Comment Actions
As mentioned on D131547, you'll need to add code to AutoUpgrade as well to transform the old get/set/create intrinsics to llvm.vector.extract/insert (plus a corresponding test).
Comment Actions
- Add intrinsics svget/svet/svcreate in AutoUpgrade.cpp file
- Add test for these intrinsic in llvm/test/Bitcode
| llvm/lib/IR/AutoUpgrade.cpp | ||
|---|---|---|
| 3901–3902 | Is the name guaranteed to start with llvm. ? If not, I would think it makes more sense to write: `if (!Name.startswith("llvm.aarch64.sve.tuple")) {`Also, should this be "tuple.get" instead of just "tuple" ? | |
| 3917 | s/Intrinsic::ID/unsigned/ ? | |
| 3928–3937 | Rather than this case relying on it being labeled as '1', can we just have: if (Name.startswith("llvm.aarch64.sve.tuple.set")) {
...
}
if (Name.startswith("llvm.aarch64.sve.tuple.create")) {
// Now parse the number after 'create'
...
}I think that clarifies the code a bit more. | |
| llvm/test/Bitcode/upgrade-aarch64-sve-intrinsics.ll | ||
| 6 | Can you add a CHECK-LABEL: @ret_svint8x2_t to this test (similar for the ones below) ? | |
| llvm/lib/IR/AutoUpgrade.cpp | ||
|---|---|---|
| 3901–3902 | If you look at line 561 in UpgradeIntrinsicFunction1 you will see this: Name.substr(5); // Strip offllvm. So I believe it is safe to assume it will start with llvm. | |
Comment Actions
Hi Carol
I think you have addressed all of the comments that were previously left here, so other than a couple of minor comments from me I think this patch looks good!
| llvm/lib/IR/AutoUpgrade.cpp | ||
|---|---|---|
| 3901–3902 | nit: I think adding a similar comment (// Strip llvm.) to this line would be helpful | |
| llvm/test/Bitcode/upgrade-aarch64-sve-intrinsics.ll | ||
| 15 | nit: can you remove #0 from the tests now that the attribute has been removed? | |
Comment Actions
-Address Kerry’s comments
Remove “#0” from upgrade-aarch64-sve-intrinsics.ll Add comment // Strip llvm
Is the name guaranteed to start with llvm. ?
If not, I would think it makes more sense to write:
`if (!Name.startswith("llvm.aarch64.sve.tuple")) {`Also, should this be "tuple.get" instead of just "tuple" ?