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

[Christopher Tetreault via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/IR/AsmWriter.cpp:659
       OS << "vscale x ";
-    OS << PTy->getNumElements() << " x ";
+    OS << PTy->getElementCount().Min << " x ";
     print(PTy->getElementType(), OS);
----------------
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) ?


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


================
Comment at: llvm/lib/IR/ConstantFold.cpp:2293
       if (VectorType *VT = dyn_cast<VectorType>(C->getType()))
-        GEPTy = VectorType::get(OrigGEPTy, VT->getNumElements());
+        GEPTy = VectorType::get(OrigGEPTy, VT);
 
----------------
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?


================
Comment at: llvm/lib/IR/Constants.cpp:1902
   if (auto *CVTy = dyn_cast<VectorType>(C->getType()))
-    assert(CVTy->getNumElements() ==
-               cast<VectorType>(DstTy)->getNumElements() &&
+    assert(cast<FixedVectorType>(CVTy)->getNumElements() ==
+               cast<FixedVectorType>(DstTy)->getNumElements() &&
----------------
I should have made this one call getElementCount


================
Comment at: llvm/lib/IR/Constants.cpp:1916
   if (auto *CVTy = dyn_cast<VectorType>(C->getType()))
-    assert(CVTy->getNumElements() ==
-               cast<VectorType>(DstTy)->getNumElements() &&
+    assert(cast<FixedVectorType>(CVTy)->getNumElements() ==
+               cast<FixedVectorType>(DstTy)->getNumElements() &&
----------------
I should have made this one call getElementCount


================
Comment at: llvm/lib/IR/Function.cpp:1331
       if (!ThisArgVecTy || !ReferenceType ||
-          (ReferenceType->getNumElements() != ThisArgVecTy->getNumElements()))
+          (ReferenceType->getElementCount() != ThisArgVecTy->getElementCount()))
         return true;
----------------
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)


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78130