Page MenuHomePhabricator

Scalable vector core instruction support + size queries
Needs ReviewPublic

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
simoll added a subscriber: simoll.Mar 8 2019, 9:00 AM
hsaito added a subscriber: hsaito.Mar 8 2019, 2:28 PM
huntergr planned changes to this revision.Mar 22 2019, 3:06 AM
vkmr added a subscriber: vkmr.Jun 14 2019, 5:10 AM
huntergr updated this revision to Diff 210089.Jul 16 2019, 6:59 AM
huntergr edited the summary of this revision. (Show Details)
huntergr set the repository for this revision to rG LLVM Github Monorepo.

Updated based on where scalable size queries were required when running with an SVE-capable loop vectorizer on several codebases (all of LNT, various flavours of SpecCPU, lots of HPC and embedded benchmarks).

Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 6:59 AM
rovka added a subscriber: rovka.Jul 23 2019, 2:22 AM

Hi Graham,

I think this patch is difficult to review. It covers many different source files with only a small unit test to check the correctness. This isn't very robust against future changes and it makes it hard to know exactly what is and isn't supported.

I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.

We would eventually end up with something like this patch, but I think that an incremental approach would have the advantage of quicker reviews with more focused discussions, more regression tests, and we would know how much was supported at any point. Does anyone else have a similar opinion?

Hi Diana,

Thanks for the comments.

I think this patch is difficult to review. It covers many different source files with only a small unit test to check the correctness. This isn't very robust against future changes and it makes it hard to know exactly what is and isn't supported.

Yeah, I was worried about that -- this is basically the size queries alone without anything actually using scalable vectors. It demonstrates roughly where changes will be needed, but doesn't actually change the surrounding code to use e.g. getElementCount instead of getNumElements.

I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.

An incremental approach sounds good; assuming nobody objects, I'll remove most of the code in this patch and just leave the core mechanism behind (in enforcing mode) and add in that test case. We can fill in the other cases as we enable codegen/acle/autovec in separate patches.

I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.

+1 to incremental approach and more tests!

This change is mostly mechanical, but you're absolutely right we need to be aware of unwanted side effects. I wrongly assumed mechanical == NFC, but this clearly isn't. Thanks for the thorough review!

I think tests need to be strict on what it should support. Not necessarily test for *all* errors, but add test for the supported cases and some negative tests for the obvious unsupported stuff.

Each step of the way, revert negative tests when new features are added (and adding more tests, too!), we can make sure it's stable and robust.

rovka added a comment.Jul 23 2019, 4:20 AM

Hi Diana,

Thanks for the comments.

I think this patch is difficult to review. It covers many different source files with only a small unit test to check the correctness. This isn't very robust against future changes and it makes it hard to know exactly what is and isn't supported.

Yeah, I was worried about that -- this is basically the size queries alone without anything actually using scalable vectors. It demonstrates roughly where changes will be needed, but doesn't actually change the surrounding code to use e.g. getElementCount instead of getNumElements.

I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.

An incremental approach sounds good; assuming nobody objects, I'll remove most of the code in this patch and just leave the core mechanism behind (in enforcing mode) and add in that test case. We can fill in the other cases as we enable codegen/acle/autovec in separate patches.

Actually my test case falls in the "replace getNumElements with getElementCount" category, rather than size queries per se. I don't think either of them is more important than the other, but you might run into the former while writing tests for the latter.

huntergr updated this revision to Diff 212548.Jul 31 2019, 4:03 AM
huntergr retitled this revision from Scalable type size queries (llvm) to Scalable vector core instruction support + size queries.
huntergr edited the summary of this revision. (Show Details)
  • Removed most of the changes in favour of reintroducing them in separate patches later with appropriate tests.
  • Added tests for core IR instructions to make sure they don't drop the scalable flag.
  • Fixed up a couple places which broke the new tests.
greened added inline comments.Jul 31 2019, 1:05 PM
llvm/include/llvm/CodeGen/ValueTypes.h
299 ↗(On Diff #212548)

Needs a comment about what this returns.

305 ↗(On Diff #212548)

Needs a comment about what this returns.

321 ↗(On Diff #212548)

Needs a comment about what this returns.

326 ↗(On Diff #212548)

Needs a comment about what this returns.

336 ↗(On Diff #212548)

Needs a comment about what this returns.

340 ↗(On Diff #212548)

Needs a comment about what this returns.

llvm/include/llvm/IR/DataLayout.h
444

Needs comments about what these return.

464

Needs a comment about what this returns.

470

Needs a comment about what this returns.

505

Needs a comment about what this returns.

510

Needs a comment about what this returns.

525

Needs a comment about what this returns.

532

Needs a comment about what this returns.

555

Needs a comment about what this returns.

560

Needs a comment about what this returns.

llvm/include/llvm/Support/MachineValueType.h
672 ↗(On Diff #212548)

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

720 ↗(On Diff #212548)

Needs a comment about what this returns.

840 ↗(On Diff #212548)

Needs a comment about what this returns.

845 ↗(On Diff #212548)

Needs a comment about what this returns.

855 ↗(On Diff #212548)

Needs a comment about what this returns.

860 ↗(On Diff #212548)

Needs a comment about what this returns.

llvm/include/llvm/Support/ScalableSize.h
43

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

102

This needs an explanatory comment.

107

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
750–753

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
555

Where is this used?

llvm/include/llvm/Support/ScalableSize.h
43

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

50

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

53

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

59

Should be implemented in terms of operator==.

66

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.

94

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
3039

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
3039

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
3039

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.Tue, Aug 27, 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.Fri, Aug 30, 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