Page MenuHomePhabricator

[SVE] Fixup calls to VectorType::getNumElements() in IR
AbandonedPublic

Authored by ctetreau on Apr 14 2020, 10:11 AM.

Details

Reviewers
efriedma
Summary

getNumElements() is going to be moved into FixedVectorType. In
preparation for this, fixup all calls to VectorType::getNumElements.
If getElementCount() can be used with no change in functionality, use
it. Otherwise cast to FixedVectorType and assume that this is correct

Diff Detail

Event Timeline

ctetreau created this revision.Apr 14 2020, 10:11 AM
Herald added a project: Restricted Project. · View Herald Transcript

My concern with switching from getNumElements() to getElementCount() is that we're essentially adding new functionality. That new functionality should have regression test coverage. If we don't do this as we go, we're never going to get reasonable regression test coverage for scalable vectors.

My concern with switching from getNumElements() to getElementCount() is that we're essentially adding new functionality. That new functionality should have regression test coverage. If we don't do this as we go, we're never going to get reasonable regression test coverage for scalable vectors.

I get where you're coming from, but I think a lot of these cases are actually equivalent. As an exercise, I can speak to each one in this patch individually, and maybe we can work out which need tests and which are NFC.

ctetreau marked 7 inline comments as done.Apr 15 2020, 11:52 AM
ctetreau added inline comments.
llvm/lib/IR/AsmWriter.cpp
660

This code is generic for all vector types. So here getNumElements() was being used correctly. We can just directly use the ElementCount.

Maybe it would be good to get the ElementCount prior to the if (isa<ScalableVectorType>(PtY)), and just do if (EC.Scalable) ?

llvm/lib/IR/ConstantFold.cpp
576–577

Suppose we have some vectors:

%a = <4 x i1> undef
%b = <4 x i8> undef
%c = <vscale x 4 x i16> undef
%d = <vscale x 4 x i32> undef

This check means to ask "do these vectors have the same number of elements?" We should have this truth table (since sameSize(l, r) = sameSize(r, l), I'll omit duplicates):

l | r | sameSize(l, r)
----------------------
a | a | true
a | b | true
a | c | false
a | d | false
b | b | true
b | c | false
b | d | false
c | c | true
c | d | true
d | d | true

With the old implementation of sameSize(l, r) = l->getNumElements() == r->getNumElements(), we would get true for all combinations. This is a bug. The getElementCount() version is correct.

The version on the right will reject vectors that were previously accepted, which will likely change the behavior of llvm. While this represents a gap in test coverage in llvm, I don't know how we can reasonably add a few tests to plug this hole. It really seems to me that scalable vectors are largely ignored by the test suite. The fix is "all tests that concern vectors should have corresponding cases that test scalable vectors"

Given plugging this hole in test coverage is hard, my questions are:

  1. Is there a reasonable test I can add to make this situation less bad?
  2. Given that the policy is that "new features need a corresponding test case", is it worth not fixing this bug?

I don't know the answer to 1. For 2, I think the bug should be fixed. I think the ship already sailed, because vscale went in without test coverage.

2293

Suppose we have some vectors:

%a = <4 x i1> undef
%d = <vscale x 4 x i32> undef

Here, we get a new vector with the same number of elements as some other vector. Using the implementation on the left, the type returned by sameShapeWithType(i8, a) is <4 x i8>, and the type returned by sameShapeWithType(i8, b) is <4 x i8>. Again, this is a bug. The version on the right (which is just a helper for VectorType::get(OrigGEPTy, VT->getElementCount())) will do the correct thing for any vector on the right hand side. The only case it doesn't handle is the case where you specifically do want to change the vector type. For this case, the old ElementCount overload still exists.

This has the same problems as the sameSize(l, R) case above: the gap in test coverage is already huge, so I have the same questions:

  1. Is there a reasonable test I can add to make this situation less bad?
  2. Given that the policy is that "new features need a corresponding test case", is it worth not fixing this bug?
llvm/lib/IR/Constants.cpp
1903–1904

I should have made this one call getElementCount

1917–1918

I should have made this one call getElementCount

llvm/lib/IR/Function.cpp
1331

this is basically the same as the == case. I think <, <=, >=, and > should probably still call getNumElements() as {2, true} > {3, false}, while possible to define in c++, is kind of nonsensical. Maybe those operators can assert that the scalablility is the same? (I think this is probably a bad idea)

llvm/lib/IR/Instructions.cpp
1977–1979

By the way, I'm doing this to preserve the original behavior. The code used to enter this branch for scalable vectors. I don't know enough about this code to judge what it should actually do for scalable vectors so I'm making it loudly fail rather than silently be buggy.

We could say certain classes of change are "trivial", I guess, in the sense that they obviously have no effect on fixed-width types. Like changing X->getNumElements() == Y->getNumElements() to use getElementCount(), or VectorType::get(X, Y->getNumElements()); -> VectorType::get(X, Y);. And then you can make the argument that you're committing these changes as a cleanup, and any effect on scalable vectors is a side-effect.

Or you could try to make the argument that we already have test coverage for everything for non-scalable types, and that should be enough to allow making these sweeping changes, since scalable types are still not production-ready anyway.

I can sympathize with both of those arguments, and I understand constructing testcases will slow you down substantially... but ultimately, if we're going to say scalable vectors are supported, we need to hit reasonable regresssion test coverage at some point. Having regression tests is a important part of making sure that developers aren't breaking scalable vector codepaths in the future. Most people working on LLVM are not going to be routinely using scalable vectors for a while; the only way they'll know if something breaks is regression tests. And the best time to add that coverage is when you're modifying the code anyway.

Maybe we can discuss this more in the meeting tomorrow.

(If you're having trouble figuring out how to exercise certain codepaths, feel free to ask.)

llvm/lib/IR/ConstantFold.cpp
576–577

This is unreachable for scalable vectors because ConstantVector and ConstantDataVector always have FixedVectorType. So it's hard to argue this specific case; you might as well just cast to FixedVectorType.

(On a side-note, if you haven't already, those values have a getType() method which returns VectorType, which should be fixed.)

llvm/lib/IR/Instructions.cpp
1977–1979

ConstantDataSequential is never scalable. (The only valid scalable constants are zero, undef, and ConstantExprs.)

llvm/lib/IR/Verifier.cpp
4908

cast<FixedVectorType>(OperandT)->getElementCount()?

ctetreau marked 2 inline comments as done.Apr 15 2020, 12:57 PM
ctetreau added inline comments.
llvm/lib/IR/ConstantFold.cpp
576–577

If ConstantVector and ConstantDataVector always have FixedVectorType, then they should probably return that directly. I can make that change. If you are aware of any other such cases, please let me know. As a general point, I am making a huge amount of changes to many disparate parts of the codebase, most of which I am not familiar with. As such, I'm going to miss these sorts of "obvious" simplifications. Please point them out to me if you see them.

As for the overall argument, it's a defense of this class of change. For specific instances, if it's never valid for the vectors to be scalable vectors, then it makes sense to not make the change.

llvm/lib/IR/Verifier.cpp
4908

oops

ctetreau marked an inline comment as done.Apr 15 2020, 1:39 PM
ctetreau added inline comments.
llvm/lib/IR/Instructions.cpp
1977–1979

ugh. Seems I misread the code. What I meant was, for cases like:

if (auto *VTy = dyn_cast<VectorType>(Ty)) {
   auto *FVTy = cast<FixedVectorType>(VTy); // immediately cast my VectorType unconditionally
   ... // stuff
}

... that I'm doing it on purpose because trying to dyn_cast to FixedVectorType would be a behavior change.

ctetreau updated this revision to Diff 257835.Apr 15 2020, 1:47 PM

rebase, address code review issues, catch stragglers

ctetreau updated this revision to Diff 258120.Apr 16 2020, 11:55 AM

catch straggler

Hi @ctetreau , @david-arm started looking through our downstream code-base to pull out some patches where we've fixed cases like the ones in this patch and for which we may already have tests.
Since there's likely to be overlap with this patch, are there any cases you've already got tests for, or are actively working on? If you have any other suggestions on how to share the effort, just let us know!

Hi @ctetreau , @david-arm started looking through our downstream code-base to pull out some patches where we've fixed cases like the ones in this patch and for which we may already have tests.
Since there's likely to be overlap with this patch, are there any cases you've already got tests for, or are actively working on? If you have any other suggestions on how to share the effort, just let us know!

I've not yet started writing tests. I've completed the refactor on my machine. I'm doing a cleanup pass now, and plan to start pushing the patches up to phabricator soon. After all the patches are up and reviewers added, I planned to evaluate what changes need tests, and what changes aren't actually worth doing.

Once that's done and the list compiled, I was going to work on getting the tests written.

I'd also like to mention that the changes in this patch are pretty representative of the changes I'm making in other modules.

Hi @efriedma, I've been looking at some our downstream tests that we have that we could upstream in this area and quite a few of them depend upon using shufflevector instructions to splat vectors and so we can't really upstream those yet. I think you're working on a splat vector implementation, right? I just wondered how it was going? Thanks!

Splats specifically should be fine. It's the other shuffles that are missing; I'll try to get back to that soon.

ctetreau abandoned this revision.Apr 23 2020, 2:11 PM

Based on the discussion in this thread, I'm going to submit a less ambitious version of this patch.