This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering] Improve legalization of scalable vector types
ClosedPublic

Authored by frasercrmck on May 7 2021, 7:43 AM.

Details

Summary

This patch extends the vector type-conversion and legalization capabilities of
scalable vector types.

Firstly, vscale x 1 types now behave more like the corresponding `vscale x
2+` types. This enables the integer promotion legalization of extended scalable
types, such as the promotion of <vscale x 1 x i5> to <vscale x 1 x i8>.

These vscale x 1 types are also now better handled by
getVectorTypeBreakdown, where what looks like older handling for 1-element
fixed-length vector types was spuriously updated to include scalable types.

Widening of scalable types is now better supported, by using INSERT_SUBVECTOR
to insert the smaller scalable vector "value" type into the wider scalable
vector "part" type. This allows AArch64 to pass and return vscale x 1 types
by value by widening.

There are still cases where we are unable to legalize vscale x 1 types, such
as where expansion would require splitting the vector in two.

Diff Detail

Unit TestsFailed

Event Timeline

frasercrmck created this revision.May 7 2021, 7:43 AM
frasercrmck requested review of this revision.May 7 2021, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 7:43 AM
craig.topper added inline comments.May 7 2021, 9:13 AM
llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll
3

These tests don't cover widening do they?

frasercrmck added inline comments.May 7 2021, 9:22 AM
llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll
3

True, they don't. To be honest I was specifically targeting the promotion but the code looks like it should handle widening okay. I'm not sure we have an RVV case where we have to widen an illegal <vscale x 1 x ty> type to a legal one.

craig.topper added inline comments.May 7 2021, 9:29 AM
llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll
3

I can't remember if the caller can handle widening scalable vectors properly. I fought with this code before when we tried to use types like <vscale x 6 x ty> for x6 segment load/stores.

Can the vectorize create these <vscale x 4 x i5> types that need promotion?

frasercrmck added inline comments.May 7 2021, 9:40 AM
llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll
3

Yeah I wouldn't be surprised. Since this patch only affects vscale x 1 types, any existing issues in widening scalable vectors are already out in the open. I think without proper tests, ot might be best to remove the claim of "allowing" widening from this patch. At least we can say it now treats vscale x 1 more similarly to the others.

I haven't looked too deeply but I believe these i5s can appear during switch generation or switch case optimizations. I don't know about the loop vectorizer, but our WFV vectorizer will happily vectorize these.

frasercrmck edited the summary of this revision. (Show Details)May 11 2021, 8:17 AM

I've tried to find a test case for vector widening but wasn't able to. I've updated the description of the patch to reflect this.

Hi @frasercrmck, thanks for working on this. We're currently investigating legalization of <vscale x 1 x eltty> vectors for SVE, so we'll be able to add some tests for the widening case.

llvm/lib/CodeGen/TargetLoweringBase.cpp
997

I found that when widening <vscale x 1 x eltty>, it shouldn't go down this code-path and hit the fatal error, but rather continue down the code path below (so that it will actually try to widening to <vscale x 2 x eltty>, <vscale x 4 x eltty>, etc. until it has found a legal one.

That said, without a way to test this having the fatal_error is fine for now, especially since we'll soon share some patches that implements the widening of these types with corresponding tests. vscale x 1 types are never legal for SVE and always need widening.

1515

Is this equivalent to EltCnt.isVector() ? (I assume we can assert that a minimum value of 0 is not allowed)

frasercrmck marked 3 inline comments as done.
  • rebase
  • add initial widening
  • support scalable vectors in widenVectorToPartType

Hi @frasercrmck, thanks for working on this. We're currently investigating legalization of <vscale x 1 x eltty> vectors for SVE, so we'll be able to add some tests for the widening case.

Hi @sdesmalen, thanks for getting in touch. I've just updated the patch to add some initial widening support for AArch64. Let me know what you think. It's very basic right now as I've found that we can't yet widen the subvector operations.

llvm/lib/CodeGen/TargetLoweringBase.cpp
997

Thanks for the input. I was wondering that whether widening would be better, yeah. I think holding off until your patch sounds reasonable.

1515

I think it's more strictly equivalent to !EltCnt.isScalar() which I was about to update to use. I think that's marginally more readable as people may wonder why we're checking isVector in what's obviously a vector method. What do you think?

llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll
3

I've added some initial widening tests for AArch64.

frasercrmck retitled this revision from [TargetLowering] Legalize "vscale x 1" types in more cases to [TargetLowering] Improve legalization of scalable vector types.May 12 2021, 3:08 AM
frasercrmck edited the summary of this revision. (Show Details)
sdesmalen added inline comments.May 12 2021, 4:56 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
619

Should this also return SDValue() if PartNumElts.isScalable() != ValueNumElts.isScalable() ?

llvm/lib/CodeGen/TargetLoweringBase.cpp
1515

I'm kind of neutral on it, but I see how this may cause some confusion. Perhaps we should consider removing the isVector() method entirely in favour of using !isScalar() at some point.

frasercrmck added inline comments.May 12 2021, 5:10 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
619

Possibly. I was thinking that technically we can widen a fixed-length Value to a scalable Part -- the same way we widen a scalable Value to scalable Part -- using INSERT_SUBVECTOR. Though only if the Part element count is known to be greater than the Value element count, so this would disallow widening v4i32 to nxv1i32, for instance. We definitely can't support widening a scalable to a fixed so we must certainly return SDValue() there.

I tried to permit these scenarios in the code, though it's hypothetical since we don't have a target that uses the "mixed" widening.

If you think that all of this is unnecessary then I'm happy to take your suggestion.

sdesmalen added inline comments.May 12 2021, 5:36 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
619

Without a specific use-case in mind, I have a slight preference to avoid widening (or possibly allowing to widen) fixed-width to scalable vectors until there is a real need for it, and until we can write a test-case. By returning SDValue, the code in getCopyTo/FromPartsVector will probably fail earlier on, rather than doing something that may be wrong. Maybe it doesn't really matter, but I guess it's easy to change this code if we do need it at some point.

frasercrmck edited the summary of this revision. (Show Details)
  • only widen vectors with equivalent scalable properties
frasercrmck marked 2 inline comments as done.May 12 2021, 6:24 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
619

Fair enough, I have a bit of a habit of doing that. I've added that check now.

sdesmalen accepted this revision.May 12 2021, 6:39 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 12 2021, 6:39 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.