Page MenuHomePhabricator

[Clang][AArch64] Use generic extract/insert vector for svget/svset/svcreate tuples
ClosedPublic

Authored by CarolineConcatto on Aug 10 2022, 12:59 AM.

Details

Summary

This patch replaces svget, svset and svcreate aarch64 intrinsics for tuple
types with the generic llvm-ir intrinsics extract/insert vector

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 12:59 AM
CarolineConcatto requested review of this revision.Aug 10 2022, 12:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 12:59 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Are you intending to AutoUpgrade the existing intrinsics in IR?

nlopes added inline comments.Aug 10 2022, 2:12 AM
clang/lib/CodeGen/CGBuiltin.cpp
9130

Please use PoisonValue here

sdesmalen added inline comments.Aug 10 2022, 5:24 AM
clang/lib/CodeGen/CGBuiltin.cpp
9093

nit: s/Emmit/Emit/

How about naming these:

  • EmitSVETupleGet
  • EmitSVETupleSet
  • EmitSVETupleCreate
9100

nit: s/unsigned int/unsigned/

9107

nit: Can this function be merged with the function above, you could pass bool IsInsert?

CarolineConcatto marked 3 inline comments as done.
  • Have EmitSVETupleSetOrGet for isTupleSet and isTupleGet
  • Address review comments

Are you intending to AutoUpgrade the existing intrinsics in IR?

@RKSimon
AFAIK we want to remove them altogether.
It is not expected that external projects depend on these intrinsics.
It is a legacy intrinsic that was introduced before we had vector.extract and vector.insert

clang/lib/CodeGen/CGBuiltin.cpp
9107

I did what you suggested. Don't know if it is much better.

Are you intending to AutoUpgrade the existing intrinsics in IR?

@RKSimon
AFAIK we want to remove them altogether.
It is not expected that external projects depend on these intrinsics.
It is a legacy intrinsic that was introduced before we had vector.extract and vector.insert

OK - not sure what aarch64 policy is, but x86 we tend to continue support for old intrinsics that could still be in IR through AutoUpgrade.cpp which mostly converts them to generics (very similar to CGBuiltin) - technically we plan to remove the oldest at some point but we've been very loathe to actually do it.

I think we usually try to do the same, if the intrinsics have been in released compilers. There is an example in https://reviews.llvm.org/D98487#change-tOTTgECYYAO5, hopefully these would be equally simple.

Matt added a subscriber: Matt.Aug 15 2022, 1:21 PM

I think we usually try to do the same, if the intrinsics have been in released compilers. There is an example in https://reviews.llvm.org/D98487#change-tOTTgECYYAO5, hopefully these would be equally simple.

We don't really have the intention in keeping compatibility for intrinsics like these, since the vector.extract/insert intrinsics are the proper way to insert/extract vectors and they have been around for quite some time now. Also these intrinsics are quite specific to the initial SVE ACLE implementation when we first upstreamed it, so it's not very likely that anyone else is using them.

clang/lib/CodeGen/CGBuiltin.cpp
9100–9112

I think you could simplify this further to:

auto *SingleVecTy = cast<llvm::ScalableVectorType>(
    TypeFlags.isTupleSet() ? Ops[2]->getType() : Ty);

unsigned I = cast<ConstantInt>(Ops[1])->getSExtValue();
Value *Idx =
    ConstantInt::get(CGM.Int64Ty, I * SingleVecTy->getMinNumElements());
  
if (TypeFlags.isTupleSet())
  return Builder.CreateExtractVector(Ty, Ops[0], Ops[2], Idx);
else
  return Builder.CreateInsertVector(Ty, Ops[0], Idx);
9121

Casting to VectorType here isn't actually necessary, because both llvm::PoisonValue::get() and Builder.CreateInsertVector() take a Type*.

9122

nit: If you cast this to llvm::ScalableVectorType you can call SrcTy->getMinNumElements() directly.

I think we usually try to do the same, if the intrinsics have been in released compilers. There is an example in https://reviews.llvm.org/D98487#change-tOTTgECYYAO5, hopefully these would be equally simple.

We don't really have the intention in keeping compatibility for intrinsics like these, since the vector.extract/insert intrinsics are the proper way to insert/extract vectors and they have been around for quite some time now. Also these intrinsics are quite specific to the initial SVE ACLE implementation when we first upstreamed it, so it's not very likely that anyone else is using them.

I'm not sure what you mean by "keeping compatibility", they just need to be replaced in the autoupdate code. If there was a release of llvm which emitted the intrinsics, then the default assumption is that someone might have bitcode that uses them. Luckily it is really simple to add the autoupgrade code in most cases.

I think we usually try to do the same, if the intrinsics have been in released compilers. There is an example in https://reviews.llvm.org/D98487#change-tOTTgECYYAO5, hopefully these would be equally simple.

We don't really have the intention in keeping compatibility for intrinsics like these, since the vector.extract/insert intrinsics are the proper way to insert/extract vectors and they have been around for quite some time now. Also these intrinsics are quite specific to the initial SVE ACLE implementation when we first upstreamed it, so it's not very likely that anyone else is using them.

I'm not sure what you mean by "keeping compatibility", they just need to be replaced in the autoupdate code. If there was a release of llvm which emitted the intrinsics, then the default assumption is that someone might have bitcode that uses them.

What I meant with 'keeping compatibility' is exactly what you described; bitcode/IR that uses the old intrinsics remaining to work with newer versions of LLVM.

My point was that these intrinsics have not been in LLVM for that long and have a very limited scope, so are unlikely to have accrued much legacy. I don't really expect a practical use-case where people have legacy SVE ACLE bitcode that they need to compile with a newer version of LLVM. So unless someone explicitly requests the compatibility, we'd rather remove them in favour of adding code that will never be used.

Is there a formal requirement that LLVM must remain backward compatible with older LLVM IR (beyond the target-independent parts)?

CarolineConcatto marked an inline comment as done.
  • Address nit in EmitSVETupleSetOrGet
CarolineConcatto marked 3 inline comments as done.Aug 17 2022, 9:41 AM

Is there a formal requirement that LLVM must remain backward compatible with older LLVM IR (beyond the target-independent parts)?

We have always done it in the past, and I don't see a good reason to change. This change is essentially for llvm 16, so we are talking about any bitcode between when SVE was added and that release. It is hard to tell how people will use bitcode up to that point and if they will expect it to continue working going forward. I think it's simpler to just add the upgrade code, than to try and argue that it is unneeded. But the upgrade code is really needed in D131548 (and D131687) where the old intrinsics are being removed.

Thanks for addressing the comments @CarolineConcatto!

Is there a formal requirement that LLVM must remain backward compatible with older LLVM IR (beyond the target-independent parts)?

We have always done it in the past, and I don't see a good reason to change. This change is essentially for llvm 16, so we are talking about any bitcode between when SVE was added and that release. It is hard to tell how people will use bitcode up to that point and if they will expect it to continue working going forward. I think it's simpler to just add the upgrade code, than to try and argue that it is unneeded. But the upgrade code is really needed in D131548 (and D131687) where the old intrinsics are being removed.

It seems that the LLVM Developer Policy provides better guidance than that.

Newer releases can ignore features from older releases, but they cannot miscompile them

Removing the intrinsics but not auto-upgrading them would mean that older IR would miscompile (the call to the intrinsic would become an actual function call). This suggests there is no freedom of choice here and we must use AutoUpgrade.

clang/lib/CodeGen/CGBuiltin.cpp
9114

nit: redundant newline, please remove.

CarolineConcatto marked an inline comment as done.Aug 18 2022, 7:16 AM
sdesmalen accepted this revision.Aug 19 2022, 3:44 AM

LGTM, thanks @CarolineConcatto. Please land D132137 before landing this one to avoid regressions on combines that previously worked on svget/svset.

clang/lib/CodeGen/CGBuiltin.cpp
9396–9397

nit: If you cast this to ScalableVectorType, you can use VTy->getMinNumElements() below on line 9385

This revision is now accepted and ready to land.Aug 19 2022, 3:44 AM
This revision was landed with ongoing or failed builds.Aug 19 2022, 4:59 AM
This revision was automatically updated to reflect the committed changes.