This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Add new VectorType subclasses
ClosedPublic

Authored by ctetreau on Apr 6 2020, 2:24 PM.

Details

Summary

Introduce new types for fixed width and scalable vectors.

Does not remove getNumElements yet so as to not break code during transition
period.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 6 2020, 2:24 PM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
ctetreau updated this revision to Diff 255496.Apr 6 2020, 2:28 PM

fix permissions

ctetreau updated this revision to Diff 255499.Apr 6 2020, 2:49 PM

Add new VectorType::get overloads

ctetreau updated this revision to Diff 255514.Apr 6 2020, 3:22 PM

Fix getPrimitiveSizeInBits

ctetreau updated this revision to Diff 255521.Apr 6 2020, 3:42 PM

Fix non-exhaustive switch

If I'm following correctly, this should apply on its own. Then you're expecting the following API changes:

  1. getNumElements() will move from VectorType to FixedVectorType. Existing code will be changed to either cast to FixedVectorType, or switch to using getElementCount().
  2. Maybe remove the default argument from VectorType::get()

Does that sound right?

llvm/include/llvm-c/Core.h
165

Adding LLVMFixedVectorTypeKind and LLVMScalableVectorTypeKind without removing LLVMVectorTypeKind does't make sense.

llvm/include/llvm/IR/DerivedTypes.h
389

"vector"

430

It seems confusing to overload get() this way.

llvm/include/llvm/IR/Type.h
76

VectorTyID should be dead.

llvm/lib/IR/Type.cpp
128

I'm not sure killing off isScalable() is productive.

efriedma added inline comments.Apr 6 2020, 3:51 PM
llvm/include/llvm/IR/Type.h
76

Oh, there's still a bunch of uses of VectorTyID. You probably need to take care of that in this patch.

Harbormaster failed remote builds in B52059: Diff 255514!
Harbormaster failed remote builds in B52064: Diff 255521!
ctetreau marked 3 inline comments as done.Apr 7 2020, 1:38 PM
ctetreau added inline comments.
llvm/include/llvm/IR/DerivedTypes.h
430

There's a bunch of code that is "I want a vector with the same shape as some other vector, but with a different element type"

Currently, there's a bunch of variants like:

auto *V2 = VectorType::Get(SomeTy, V1->getNumElements());
auto *V3 = VectorType::Get(SomeTy, V4->getElementType());

Really, for all variants of this operation, the first is potentially buggy if v1 is scalable, and it can always be replaced with the second and be correct (unless you are specifically trying to get a fixed width vector that has the same minimum number of elements as some potentially scalable vector, but that's a strange special case and I'd ask in code review that the author specifically pass false). But really, the quantity that you get out of the second argument is always noise. This change makes the following equivalent:

auto *V2 = VectorType::Get(SomeTy, V1->getElementType());
auto *V3 = VectorType::Get(SomeTy, V1);
assert(V2->getType() == V3->getType());

If V1 was scalable, then V2 is scalable, and vice versa. If you like, I can improve the documentation for this function, but I think it adds value.

llvm/include/llvm/IR/Type.h
76

Since this thing is exposed from the C api, it might break external code to remove it.

However, it is the design that it should never be possible to construct a base VectorType. Maybe it makes sense to delete it here, but leave the type in the C api?

llvm/lib/IR/Type.cpp
128

I'm still in the early stages of this, but if I have an instance of ScalableVectorType, then this is always true, and vice versa. It's kind of a pointless function. Usages of it tend to look like this:

if (isa<VectorType>(Ty) && cast<VectorType>(Ty)->isScalable())
   doStuff(Ty);

If it ends up being a ton of work, I'll leave it

efriedma added inline comments.Apr 7 2020, 3:52 PM
llvm/include/llvm/IR/Type.h
76

Whether or not we actually remove LLVMVectorTypeKind from the enum, it's a breaking change from the C API's perspective if vector types don't have the right ID. So we have two options: either make the breaking change to the C API, or don't expose the Scalable/Fixed split via the C API at all (and explicitly remap the internal IDs in LLVMGetTypeKind).

Either way, that isn't a reason to keep around the C++ VectorTyID; we don't expose that directly anyway.

ctetreau marked an inline comment as done.Apr 7 2020, 4:29 PM
ctetreau added inline comments.
llvm/include/llvm/IR/Type.h
76

Fair enough, I'll get rid of it

ctetreau updated this revision to Diff 256100.Apr 8 2020, 1:04 PM

adddress code review issues

Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2020, 1:04 PM

If I'm following correctly, this should apply on its own. Then you're expecting the following API changes:

  1. getNumElements() will move from VectorType to FixedVectorType. Existing code will be changed to either cast to FixedVectorType, or switch to using getElementCount().
  2. Maybe remove the default argument from VectorType::get()

Does that sound right?

This is basically the plan:

  1. Add the new types and functions
  2. Fix up usages of getNumElements()
  3. move getNumElements() and getMinNumElements() into the derived classes
ctetreau updated this revision to Diff 256101.Apr 8 2020, 1:17 PM

Fix permissions

ctetreau updated this revision to Diff 256129.Apr 8 2020, 3:42 PM

Move getMinNumElements to ScalableVectorType. There's no reason for it to be in base VectorType

Looks right, generally.

llvm/include/llvm-c/Core.h
179

The C API is officially not ABI-stable anymore across versions, No reason to have a gap like this.

llvm/lib/ExecutionEngine/ExecutionEngine.cpp
1011 ↗(On Diff #256129)

?

llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
481 ↗(On Diff #256129)

Please fix this to use isa<> while you're here.

ctetreau marked an inline comment as done.Apr 9 2020, 12:23 PM
ctetreau added inline comments.
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
1011 ↗(On Diff #256129)

clang-format-diff must have done this.

Thanks for this patch @ctetreau! Overall looks great to me, just two little nits.

llvm/include/llvm/IR/DerivedTypes.h
434

Do you need the method on line 432 if you have this one (that takes a const VectorType *Other) ?

llvm/lib/ExecutionEngine/ExecutionEngine.cpp
1109 ↗(On Diff #256129)

This code doesn't really work for scalable vectors. Assuming you don't want to change that in this patch, Is it worth putting a FIXME here?

ctetreau marked 2 inline comments as done.Apr 9 2020, 3:47 PM
ctetreau added inline comments.
llvm/include/llvm/IR/DerivedTypes.h
434

It shouldn't be needed, I'll remove it.

llvm/lib/ExecutionEngine/ExecutionEngine.cpp
1109 ↗(On Diff #256129)

This will have to be changed when getNumElements() moves into FixedVectorType. I'll get it then.

Likely, the plan will be to just have the scalable vector branch assert. My overall strategy is to just assume all calls to getNumElements are correct, cast to FixedVectorType instead of VectorType, and just fix enough to make the tests pass.

There's a lot of code that calls getNumElements, and the overall goal is to force everybody to clean their own houses.

ctetreau updated this revision to Diff 256435.Apr 9 2020, 3:50 PM

address code review issues

ctetreau marked an inline comment as done.Apr 14 2020, 9:20 AM
ctetreau added inline comments.
llvm/include/llvm/IR/DerivedTypes.h
430

In this example, "getElementType" should be "getElementCount", but I assume you get the idea...

sdesmalen added inline comments.Apr 14 2020, 2:30 PM
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
411 ↗(On Diff #257138)

should this be !isa<FixedVectorType>(ArgType) (i.e. negated)?

ctetreau updated this revision to Diff 257542.Apr 14 2020, 3:51 PM

address code review issues

ctetreau marked 2 inline comments as done.Apr 14 2020, 3:57 PM
sdesmalen accepted this revision.Apr 20 2020, 2:15 PM

Thanks for all the changes, this patch LGTM!

This revision is now accepted and ready to land.Apr 20 2020, 2:15 PM
ctetreau updated this revision to Diff 259044.Apr 21 2020, 10:14 AM

Fixup canLosslesslyBitCastTo to use FixedVectorType again

ctetreau updated this revision to Diff 259104.Apr 21 2020, 2:16 PM

remove unused variable

ctetreau updated this revision to Diff 259114.Apr 21 2020, 3:28 PM

Catch stragglers in C api

ctetreau updated this revision to Diff 259126.Apr 21 2020, 4:29 PM

split C api stuff into separate commit. Do bare minimum here

This revision was automatically updated to reflect the committed changes.