This is an archive of the discontinued LLVM Phabricator instance.

[ValueTypes] Rename MVT::getVectorNumElements() to MVT::getVectorMinNumElements(). Fix some misuses of getVectorNumElements()
ClosedPublic

Authored by craig.topper on May 11 2021, 11:01 AM.

Details

Summary

getVectorNumElements() returns a value for scalable vectors
without any warning so it is effectively getVectorMinNumElements().
By renaming it and making getVectorNumElements() forward to
it, we can insert a check for scalable vectors into getVectorNumElements()
similar to EVT. I didn't do that in this patch because there are still more
fixes needed, but I was able to temporarily do it and passed the RISCV
lit tests with these changes.

The changes to isPow2VectorType and getPow2VectorType are copied from EVT.

The change to TypeInfer::EnforceSameNumElts reduces the size of AArch64's isel table.
We're now considering SameNumElts to require the scalable property to match which
removes some unneeded type checks.

This was motivated by the bug I fixed yesterday in 80b9510806cf11c57f2dd87191d3989fc45defa8

Diff Detail

Event Timeline

craig.topper created this revision.May 11 2021, 11:01 AM
craig.topper requested review of this revision.May 11 2021, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 11:01 AM
craig.topper edited the summary of this revision. (Show Details)May 11 2021, 11:03 AM
sdesmalen accepted this revision.May 11 2021, 2:00 PM

Thanks, this looks like sensible changes to me! Do you have a ballpark figure of how many tests currently fail when adding a check to getVectorNumElements? The fact that the RISCV lit-tests already pass sounds promising.
I've taken the liberty to accept the patch, but maybe just wait a day or two to give other reviewers a chance to take a look as well.

This revision is now accepted and ready to land.May 11 2021, 2:00 PM

Thanks, this looks like sensible changes to me! Do you have a ballpark figure of how many tests currently fail when adding a check to getVectorNumElements? The fact that the RISCV lit-tests already pass sounds promising.
I've taken the liberty to accept the patch, but maybe just wait a day or two to give other reviewers a chance to take a look as well.

There are failures coming from computeRegisterProperties on multiple targets that cause a very high test fallout.

craig.topper updated this revision to Diff 344593.EditedMay 11 2021, 4:37 PM

Fix a few more issues in some target's getPreferredAction.

Majority of remaining issues are in GlobalISel on AArch64
due to LLT not handling scalable vectors. Failing cases are
LLT is constructed from getMinimalRegClassLLT.

There are two additional failures
test/Analysis/CostModel/AArch64/sve-intrinsics.ll
Transforms/LoopVectorize/AArch64/first-order-recurrence.ll
Both may be from getExtractSubvectorOverhead in TTI

frasercrmck accepted this revision.May 12 2021, 1:14 AM

LGTM, with some optional nits.

llvm/lib/Target/X86/X86ISelLowering.cpp
2104

You could probably also express this (and elsewhere) with !VT.getVectorElementCount().isScalar() as is done in some other places, but I'm undecided which is better

llvm/utils/TableGen/CodeGenDAGPatterns.cpp
699

I don't know if there's a way of making this a bit cleaner without impacting TableGen performance. Would ElementCount with getNull for scalars work? T.isVector() ? T.getVectorElementCount() : ElementCount::getNull()

craig.topper added inline comments.May 12 2021, 7:40 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
2104

Wouldn't !VT.getVectorElementCount().isScalar() be equivalent to (isScalableVector || VT.getVectorNumElements() != 1). I don't think that's what we want here.

frasercrmck added inline comments.May 12 2021, 8:05 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
2104

Oh yes I was thinking of a case I was dealing with where I *do* want to allow widening for the vscale x 1 scalable vectors (D102073). I didn't notice that you wanted something different, sorry.

With that in mind -- and maybe it's academic because X86, PPC, Hexagon etc won't encounter scalable types -- perhaps !isScalar is more consistent between the scalable types?