Page MenuHomePhabricator

Scalable vector core instruction support + size queries
ClosedPublic

Authored by huntergr on Oct 11 2018, 6:59 AM.

Details

Summary

Implements basic size queries to support scalable vectors in LLVM IR.

Adds simple tests for IR instructions usable with scalable vector types to ensure that they can be parsed and printed back out without dropping the scalable flag; used the size queries and getElementCount in a couple of places to get it working.

Adds a few scalable size query calls in codegen and tablegen to keep existing tests working, including the recent SVE calling convention test.

Adds checks in various backends which don't support scalable vectors to skip over them when registering legalization and DAG actions.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
greened added inline comments.Jul 31 2019, 1:05 PM
llvm/include/llvm/IR/DataLayout.h
509

Needs a comment about what this returns.

llvm/include/llvm/Support/MachineValueType.h
674

Not sure why the other methods here don't have comments but we should probably have one here to say what this does.

722

Needs a comment about what this returns.

836

Needs a comment about what this returns.

841

Needs a comment about what this returns.

842

Needs a comment about what this returns.

847

Needs a comment about what this returns.

llvm/include/llvm/Support/ScalableSize.h
41

Needs comments about what this is and what the fields represent.

100

This needs an explanatory comment.

105

Needs a comment about what this returns.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
4612 ↗(On Diff #212548)

This comment should probably be moved right before the if below.

llvm/lib/IR/DataLayout.cpp
749–752

I might help to clarify that this comment only applies to scalable types, at least as far as I understand this changes here.

llvm/lib/Target/Hexagon/HexagonISelLowering.cpp
1438 ↗(On Diff #212548)

What about other backends? Do they need similar checks (both here and below)?

rovka added a comment.Aug 1 2019, 5:00 AM

Several more general comments:

  • Should all the getXSize assert when called on a scalable type? I see that for MVT::getSizeInBits and for Type::getPrimitiveSize, but not for the others. This should also be made clear in the comments for each of them.
  • Great test for the IR, thanks!
  • I don't see any test for the CodeGen stuff though. Is it possible to add one? (If not, maybe add the changes to EVT etc when we can actually test them).
  • Ditto for TableGen (or if that's too difficult/hairy to test, just update the commit message to explain exactly why the change belongs in this patch).
llvm/include/llvm/CodeGen/ValueTypes.h
214 ↗(On Diff #212548)

Why does this one need to change? We're only looking at MinSize anyway, isn't getSizeInBits good enough?

llvm/include/llvm/IR/DataLayout.h
504

Where is this used?

llvm/include/llvm/Support/ScalableSize.h
41

Maybe make this a class, so people can't just get at the MinSize directly? Otherwise, it doesn't make much sense to have accessors for the fields (and especially getFixedSize, which can be completely circumvented via direct access).

48

Isn't this already deleted because we have a constructor just above?

51

I think it's more canonical to say return std::tie(MinSize, Scalable) == std::tie(RHS.MinSize, RHS.Scalable).

57

Should be implemented in terms of operator==.

64

Needs a comment saying that you can't compare scalable sizes and fixed sizes.
You can simplify the code by replacing the check for Scalable with an assert and removing the unreachable at the end.
Also, all the other comparison operators below should be implemented in terms of this.

92

Would be nice to also have a non-member one for symmetry (so you can write 2 * SomeSize, not just SomeSize * 2).

llvm/lib/CodeGen/ValueTypes.cpp
106 ↗(On Diff #212548)

Should also assert that it's not called on a scalable type.

Thanks for the reviews; I left a couple of inline comments, and will make the requested changes.

llvm/include/llvm/CodeGen/ValueTypes.h
214 ↗(On Diff #212548)

getSizeInBits will assert (actually, the underlying MVT version for it will) for a scalable vector type, so can't be used.

However, I introduced the 'getMinSizeInBits' function after writing this, so it should be used here.

llvm/lib/Target/Hexagon/HexagonISelLowering.cpp
1438 ↗(On Diff #212548)

I'll take a look, but the Hexagon backend was the only one which asserted when running make check-all.

huntergr updated this revision to Diff 213589.Aug 6 2019, 5:54 AM
huntergr edited the summary of this revision. (Show Details)
  • Added comments explaining the new methods
  • Added tests for the MVT/EVT/DataLayout interfaces
  • Refactored ScalableSize class operators to build on '==' and '<'
  • Added checks in various backends that don't support scalable vectors to prevent legalizing operations involving scalable MVTs
huntergr marked 34 inline comments as done.Aug 6 2019, 5:59 AM

Thanks @huntergr for working on this!

This patch can probably be split into two separate patches, which make them easier to review;

  • One that fixes the Targets to ignore scalable types (see comment)
  • Another one that adds the interface for scalable size queries.

On the interfaces itself, I personally find ScalableSize a bit of a misnomer (see comment for other suggestions) because it describes both scalable and fixed-width sizes, which is not what the name suggests. But perhaps more importantly, my concern is that with the interface as defined in this patch, there is little incentive to move the LLVM codebase to work on ScalableSize. Especially for new code that gets added, the interface should let developers make a conscious decision whether their code is fixed-width only, scalable-width only, or agnostic to this property. I would much rather see getSizeInBits() return a ScalableSize object, which in turn only has getFixedSize() and getMinScalableSize() as query methods. If the code is agnostic to whether the size of an object is fixed or scalable, the code should simply operate on the ScalableSize object itself, rather than operating on MinSize.

I realise that the entire code-base currently expects 'unsigned' or 'int', but this can easily be fixed by adding a (hopefully temporary) overloaded cast operator to the struct that produces a scalar value, like:

/// Casts to a uint64_t if this is a fixed-width size.
///
/// NOTE: This interface is obsolescent and will be removed
/// in a future version of LLVM in favour of getFixedSize().
operator uint64_t() const {
  assert(isFixed() && "Expected fixed size data type");
  return getFixedSize();
}

We can then update the codebase piece by piece, incrementally making use of ScalableSize or its interfaces. When all that is done, the codebase should compile without errors when building for LLVM_TARGETS_TO_BUILD=AArch64 after we remove the overloaded operator.

llvm/include/llvm/Support/ScalableSize.h
83

All the comparison operators assert that the types are both fixed-width or both scalable.
Is there value in also adding the following interfaces?

// Returns true if A is known to be at least as big as B, e.g.
// If A is scalable, and B is not, returns A.MinSize >= B.MinSize
// If A is not scalable, and B is, returns false regardless of MinSize.
bool knownGreaterOrEqual(const ScalableSize &B) const { ... }
// or alternatively: "atLeastAsBigAs"?

and

// Returns true if A is known to always be larger than B, e.g.
// If A is scalable, and B is not, returns A.MinSize > B.MinSize
// If A is not scalable, and B is, returns false regardless of MinSize.
bool knownGreater(const ScalableSize &B) const { ... }
120

I think this only really makes sense in the context of scalable vectors, and don't think we want to expose it as a property for both fixed/scalable vectors, e.g.:

uint64_t getMinSize() const {
  assert (isScalable() && "MinSize only makes sense in the context of a scalable vector,"
                          " use getFixedSize() instead");
  return MinSize;
}

For cases where we need to use it in a comparison (to know if it is at least a given number of bytes) we can do this in a separate comparison interface.

125

bool ScalableSize::isScalable() proves to me that the name ScalableSize is a misnomer. If you have a ScalableSize object, I'd expect it to always represent a scalable size.

Can we rename this to something more generic like ObjectSize or PrimitiveSize? (suggestions welcome)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
9177 ↗(On Diff #213589)

OutputArg::PartOffset is defined explicitly as a byte offset, so until we change that, this should be getFixedSize().

llvm/lib/IR/Instructions.cpp
3053

MinSize suggests that it could be larger at runtime, so SrcBits.getMinSize() == 0 would always be true. If you instead overload operator bool() which checks for Fixed size and Scalable size to be 0, you can rewrite this as (!SrcBits || !DestBits) .

llvm/lib/Target/X86/X86ISelLowering.cpp
721 ↗(On Diff #213589)

Adding MVT::fixedwidth_[integer_|fp_]vector_valuetypes() seems like a more natural interface than skipping based on isScalableVector(). There already seem to be iterators for MVT::(integer_|fp_)_scalable_vector_valuetypes.

You can do that in a separate patch, so this patch can focus solely on scalable size queries.

huntergr added inline comments.Aug 9 2019, 3:15 AM
llvm/include/llvm/Support/ScalableSize.h
83

If you can think of an immediate use case, sure.

Otherwise I'd leave it to another patch which requires that information.

120

This is used for the alignment checks at the moment, where we just need to know the minimum for scalable and exact for fixed.

I'd rather not have code that looks like

unsigned Align;
if (VTy->isScalable())
  Align = getMinSize(VTy);
else
  Align = getFixedSize(VTy);

in several places, when it could be done with one.

We can certainly bikeshed the names, though, and come up with a more explicit one which acknowledges it can represent a known quantity from fixed or scalable vectors.

I would also need to convert a larger part of the codebase to use scalable types in order to use this right now, and I've already had pushback on the size of the changes.

125

Yeah, I figured it needed a rename, but plain 'Size' was likely to cause problems. ObjectSize or TypeSize might work.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
9177 ↗(On Diff #213589)

Then we would assert when running unit tests.

I'm trying to juggle between an enormous patch that fixes everything 'properly', and something which gets us partway and keeps us running but needs fixes elsewhere later (which seemed to be what other reviewers would prefer).

I could certainly add a comment here to explain that changes are needed, though.

llvm/lib/IR/Instructions.cpp
3053

No. 0 * vscale is still 0 for any value of vscale.

The ScalableSize class says the Scalable flag indicates the total size is an integer multiple of the known minimum size.

I suspect a better way of doing this test would be to explicitly check for the elements of one being pointer type and not the other, instead of relying on a hack with the size.

llvm/lib/Target/X86/X86ISelLowering.cpp
721 ↗(On Diff #213589)

Yeah, I thought of that after I'd submitted the patch. Will do.

Thanks @huntergr for working on this!

This patch can probably be split into two separate patches, which make them easier to review;

  • One that fixes the Targets to ignore scalable types (see comment)
  • Another one that adds the interface for scalable size queries.

Ok, will do.

On the interfaces itself, I personally find ScalableSize a bit of a misnomer (see comment for other suggestions) because it describes both scalable and fixed-width sizes, which is not what the name suggests. But perhaps more importantly, my concern is that with the interface as defined in this patch, there is little incentive to move the LLVM codebase to work on ScalableSize. Especially for new code that gets added, the interface should let developers make a conscious decision whether their code is fixed-width only, scalable-width only, or agnostic to this property. I would much rather see getSizeInBits() return a ScalableSize object, which in turn only has getFixedSize() and getMinScalableSize() as query methods. If the code is agnostic to whether the size of an object is fixed or scalable, the code should simply operate on the ScalableSize object itself, rather than operating on MinSize.

See some of the inline comments -- there are a few places where we'd just end up duplicating code if used that way. The names can certainly be improved for clarity, though, and we could state that the (scalable|fixed)-only interfaces should be used in preference to a joint one.

I realise that the entire code-base currently expects 'unsigned' or 'int', but this can easily be fixed by adding a (hopefully temporary) overloaded cast operator to the struct that produces a scalar value, like:

/// Casts to a uint64_t if this is a fixed-width size.
///
/// NOTE: This interface is obsolescent and will be removed
/// in a future version of LLVM in favour of getFixedSize().
operator uint64_t() const {
  assert(isFixed() && "Expected fixed size data type");
  return getFixedSize();
}

We can then update the codebase piece by piece, incrementally making use of ScalableSize or its interfaces. When all that is done, the codebase should compile without errors when building for LLVM_TARGETS_TO_BUILD=AArch64 after we remove the overloaded operator.

I'm fine with that approach if others approve; I was trying to minimize the overall impact on the codebase though.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
9177 ↗(On Diff #213589)

A followup thought on this; would it be preferable to split the EVT/MVT side of this patch out into a separate one, and fix this properly there?

See some of the inline comments -- there are a few places where we'd just end up duplicating code if used that way. The names can certainly be improved for clarity, though, and we could state that the (scalable|fixed)-only interfaces should be used in preference to a joint one.

Yes, adding some words to state that in the description of the method would be good. There are indeed cases where MinSize is needed directly (like in DAGCombiner.cpp where it uses MaximumLegalStoreInBits = MinSize), but such a case is very explicit (it needs to specifically query the known part of the size), which I think is not very common. We should strive to reduce those uses as much as possible. For Alignment for example, I think we can use a separate method (see comment).

I realise that the entire code-base currently expects 'unsigned' or 'int', but this can easily be fixed by adding a (hopefully temporary) overloaded cast operator to the struct that produces a scalar value, like:

/// Casts to a uint64_t if this is a fixed-width size.
///
/// NOTE: This interface is obsolescent and will be removed
/// in a future version of LLVM in favour of getFixedSize().
operator uint64_t() const {
  assert(isFixed() && "Expected fixed size data type");
  return getFixedSize();
}

We can then update the codebase piece by piece, incrementally making use of ScalableSize or its interfaces. When all that is done, the codebase should compile without errors when building for LLVM_TARGETS_TO_BUILD=AArch64 after we remove the overloaded operator.

I'm fine with that approach if others approve; I was trying to minimize the overall impact on the codebase though.

Great! Looking forward to hear how others feel about it.

llvm/include/llvm/Support/ScalableSize.h
83

One use-case is in DAGCombiner.cpp, where it currently has:

VT.getMinSizeInBits() >= MaximumLegalStoreInBits

which would become:

VT.getSizeInBits().isKnownGreaterOrEqual(MaximumLegalStoreInBits)

This removes another use-case for using MinSize directly.

120

For Alignment, because it is so common, I think it is worth adding a separate method:

unsigned getNaturalAlignment() const { return (unsigned) getKnownSize(); };
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
9177 ↗(On Diff #213589)

Right, I see. At some point this class should probably use StackOffset to represent such offsets. For now, I'd say this line warrants a "FIXME" comment.

9177 ↗(On Diff #213589)

A followup thought on this; would it be preferable to split the EVT/MVT side of this patch out into a separate one, and fix this properly there?

I think so.

llvm/lib/IR/Instructions.cpp
3053

Perhaps I am a little pedantic with how I read this, because I expect the minimum size of every object to be 0, always :)

So perhaps instead of using the name MinSize, a name like KnownSize would be better suited. (which would also nicely match with the isKnownGreaterOrEqual() suggestion for DAGCombiner.cpp).

llvm/lib/Target/X86/X86ISelLowering.cpp
721 ↗(On Diff #213589)

Thanks!

Created D66339 to split out the code to skip scalable vector types in other backends.

huntergr updated this revision to Diff 217354.Aug 27 2019, 3:40 AM
  • Split out backend code into separate patches
  • Renamed 'Min' to 'KnownMin' in method names.
  • Added a few more comments.

I tried to replace the comparison of minimum sizes to 0 in Instructions.cpp (for the bitcast checks), but that statement represents quite a lot. Could be scalar pointers, could be a vector of pointers (both of which I checked for in experiments), but there are several other types which report a size of 0 and which are checked for in some unit tests, so the number of checks ended up being fairly substantial. I've left it alone for now, but if reviewers would prefer I extract a method to explicitly check for all conditions represented by the size check I can do so.

Does anyone like Sander's suggestion to make ScalableSize (or whatever we end up naming it) the return value for all size queries and provide an overloaded cast operator to transparently work with existing code comparing against unsigned values? Or is it preferable to keep the current split?

rovka added a comment.Aug 30 2019, 1:25 AM

Does anyone like Sander's suggestion to make ScalableSize (or whatever we end up naming it) the return value for all size queries and provide an overloaded cast operator to transparently work with existing code comparing against unsigned values? Or is it preferable to keep the current split?

+1 from me, I definitely like Sander's suggestion. I thought we were heading towards ScalableSize-only functions anyway and that this was just a temporary path until we cleaned up the codebase from unsigned. Using a cast operator from the start seems like an even better way of getting there, since that way at least we hopefully won't get that many new uses of unsigned in the interim.

+1 from me, I definitely like Sander's suggestion. I thought we were heading towards ScalableSize-only functions anyway and that this was just a temporary path until we cleaned up the codebase from unsigned. Using a cast operator from the start seems like an even better way of getting there, since that way at least we hopefully won't get that many new uses of unsigned in the interim.

+1

huntergr updated this revision to Diff 221283.Sep 23 2019, 4:22 AM
  • Changed existing interface to return ScalableSize objects and added a (mostly) transparent conversion, as per Sander's suggestion.
  • Removed new interfaces for DataLayout and Type.
  • Fixed cases where the transparent conversion doesn't quite work (e.g. std::max/min, where the types must be the same)

I suspect 'ScalableSize' is the wrong term now; 'TypeSize' may be better. Thoughts?

Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 4:22 AM
rovka added a comment.Sep 24 2019, 2:21 AM

I suspect 'ScalableSize' is the wrong term now; 'TypeSize' may be better. Thoughts?

I agree, TypeSize sounds better. Maybe we can replace the public constructor with 2 static methods, TypeSize::Fixed(Size) and TypeSize::Scalable(Size), so we don't always have to spell out /* Scalable =*/.

llvm/include/llvm/IR/DataLayout.h
457

We already overload operator /, why not overload + as well so we don't have to change the body of this method?

490

Can we add a version of alignTo that works with ScalableSize instead?

655

Maybe just return VTy->getElementCount() * getTypeSizeInBits(VTy->getElementType()).getFixedSize().

huntergr added inline comments.Oct 1 2019, 5:09 AM
llvm/include/llvm/IR/DataLayout.h
457

Scaling a size with * or / has a clear meaning to me, since it's independent of vscale; getting a vector that's half the size or four times larger just works.

Using + (or -) on the other hand doesn't seem to be as clear; I wasn't sure if a standalone int should be automatically treated as being the same as the TypeSize, or always considered Fixed. If we try for the former I can imagine quite a few bugs arising.

I could add a roundBitsToNearestByteSize method to move the arithmetic elsewhere if that would be acceptable?

655

There's no support for generating a TypeSize from an ElementCount in that way; is that an interface you feel is useful?

(I'll certainly change the getKnownMinSize to getFixedSize though, since we're just referring to a scalar)

huntergr updated this revision to Diff 222593.Oct 1 2019, 5:15 AM
  • Renamed ScalableSize to TypeSize, including header name.
  • added alignTo function that takes and returns a TypeSize. I wasn't sure if this should be added to MathExtras.h where the other variants live, so just kept it in TypeSize.h for now
rovka accepted this revision.Oct 2 2019, 3:01 AM
rovka marked 2 inline comments as done.

This looks good to me, maybe wait a while to see if anyone else has any further comments.

llvm/include/llvm/IR/DataLayout.h
457

You're right, + on TypeSizes would be confusing. This looks ok as-is then, no need to fiddle with it more.

655

Actually, no, now that I think about it a bit more it might be clearer to spell it out this way.

llvm/include/llvm/Support/TypeSize.h
123

Microscopic nit: punctuation.

144

Ditto.

149

Ditto.

This revision is now accepted and ready to land.Oct 2 2019, 3:01 AM
sdesmalen accepted this revision.Oct 2 2019, 5:00 AM

Thanks @huntergr , I think this interface looks really nice. LGTM!

llvm/include/llvm/IR/DataLayout.h
456

nit: use TypeSize instead of auto.

622

Nice, I like the interface of TypeSize::Fixed() for fixed-size types.

llvm/lib/IR/Instructions.cpp
2986–2987

nit: auto -> TypeSize ?

3048–3049

nit: auto -> TypeSize ?

This revision was automatically updated to reflect the committed changes.

Hmm, forgot to add the last round of minor fixes before committing. Sorry about that, will push them as well.