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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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



================
Comment at: llvm/lib/IR/ConstantFold.cpp:576
       DestTy->isVectorTy() &&
-      cast<VectorType>(DestTy)->getNumElements() ==
-          cast<VectorType>(V->getType())->getNumElements()) {
+      cast<VectorType>(DestTy)->getElementCount() ==
+          cast<VectorType>(V->getType())->getElementCount()) {
----------------
ctetreau wrote:
> 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.
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.)


================
Comment at: llvm/lib/IR/Instructions.cpp:1977
   if (const auto *CDS = dyn_cast<ConstantDataSequential>(Mask)) {
-    unsigned V1Size = cast<VectorType>(V1->getType())->getNumElements();
-    for (unsigned i = 0, e = MaskTy->getNumElements(); i != e; ++i)
+    unsigned V1Size = cast<FixedVectorType>(V1->getType())->getNumElements();
+    for (unsigned i = 0, e = cast<FixedVectorType>(MaskTy)->getNumElements();
----------------
ctetreau wrote:
> 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.
ConstantDataSequential is never scalable.  (The only valid scalable constants are zero, undef, and ConstantExprs.)


================
Comment at: llvm/lib/IR/Verifier.cpp:4908
     if (auto *OperandT = dyn_cast<VectorType>(Operand->getType())) {
-      NumSrcElem = OperandT->getNumElements();
+      NumSrcElem = cast<FixedVectorType>(OperandT)->getElementCount();
     }
----------------
`cast<FixedVectorType>(OperandT)->getElementCount()`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78130/new/

https://reviews.llvm.org/D78130