Page MenuHomePhabricator

[AArch64]Remove svget/svset/svcreate from llvm
ClosedPublic

Authored by CarolineConcatto on Aug 10 2022, 1:10 AM.

Details

Summary

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:10 AM
CarolineConcatto requested review of this revision.Aug 10 2022, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:10 AM
sdesmalen added inline comments.Aug 15 2022, 3:46 AM
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.
Can you change this code to be:

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 ?

  • Change order of vector.extract instcombine
CarolineConcatto marked 2 inline comments as done.Aug 15 2022, 6:27 AM
CarolineConcatto added inline comments.
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.

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.

  • this test currently calls @llvm.aarch64.sve.st4
  • this test currently uses attributes #0 = { "target-features"="+sve" }
  • this test current has target triple = "aarch64-unknown-linux-gnu"
Matt added a subscriber: Matt.Aug 15 2022, 1:21 PM
CarolineConcatto marked an inline comment as done.

Remove aarch64 dependency in opts-tuples-extract-intrinsic.ll test file

  • nit in opts-tuples-extract-intrinsic.ll

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

sdesmalen added inline comments.Aug 17 2022, 8:37 AM
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:

  1. extract.vector(insert.vector(InsertTuple, InsertValue, Idx), Idx) --> InsertValue
  2. extract.vector(insert.vector(InsertTuple, InsertValue, Idx1), Idx2) --> extract.vector(InsertTuple, Idx2)
  3. And also add a negative test where the extracted vector-size != inserted vector-size.

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
}
CarolineConcatto marked an inline comment as done.

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.
But just in case this was the original test we had for svget/svset intrinsics.

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!

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

  • Add intrinsics svget/svet/svcreate in AutoUpgrade.cpp file
  • Add test for these intrinsic in llvm/test/Bitcode
sdesmalen added inline comments.Thu, Sep 8, 2:53 AM
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) ?
(can you just use the update_test_checks.py script for this?)

CarolineConcatto marked an inline comment as done.
  • Address comments for sve.tuple.create
  • Add check label in the tests
CarolineConcatto marked 2 inline comments as done.Thu, Sep 8, 5:55 AM
CarolineConcatto added inline comments.
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.

kmclaughlin accepted this revision.Thu, Sep 22, 7:02 AM
kmclaughlin added a subscriber: kmclaughlin.

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?

This revision is now accepted and ready to land.Thu, Sep 22, 7:02 AM

-Address Kerry’s comments

Remove “#0” from upgrade-aarch64-sve-intrinsics.ll
Add comment  // Strip llvm
This revision was automatically updated to reflect the committed changes.