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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked 2 inline comments as done.
ctetreau added inline comments.


================
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()) {
----------------
efriedma wrote:
> 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.)
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78130