[SVE] Remove calls to VectorType::getNumElements from ExecutionEngine

[ctetreau]

The goal for this patch is to remove all invalid calls to VectorType::getNumElements with as few behavior changes as possible. If some code previously called getNumElements via a base VectorType pointer, we are assuming that it meant to treat the vector as a fixed width vector, and are instead unconditionally casting to FixedVectorType. Any test failures that arise from this change are cases where the assumption wasn't correct, and these are being handled on a case by case basis.

We're trying to avoid behavior changes of the form "this branch used to be taken for scalable vectors, now it isn't" because 1) There are a ton of such cases, and if we had to write tests for all of them right now this work would never get done and 2) A lot of these cases are actually impossible to trigger for scalable vectors (as of today), so we can't actually write tests for them. In general anything that has to do with constants is hard to test with scalable vectors because there are only a few scalable vectors that it's actually possible to construct constants for today. This means that some very strange code will result. Things like:

if (auto *VTy = dyn_cast<VectorType>(Ty)) {
   unsinged NumElts = cast<FixedVectorType>(VTy);

If we just dyn_cast to FixedVectorType, then a branch that used to be taken for scalable vectors will now be skipped, and this is a behavior change that requires a new test. It is my hope that this code will look very strange to people familiar with the code in question, and that it will get fixed over time. If this does crash, it will be very obvious that the cast probably isn't correct. People are unlikely to see this and ask themselves "is VTy actually supposed to be a FixedVectorType? Did *I* break this?" We are also intentionally avoiding some "obvious" fixes like auto *VTy = VectorType::get(SomeTy, SomeVTy->getNumElements()) => auto *VTy = VectorType::get(SomeTy, SomeVTy->getElementCount()). While it seems obvious that the second form should be fine, it is technically a behavior change that requires a new test, and this sort of thing happens a lot. As with the dyn_cast issue, if we had to write tests for every instance, this work would never get done.

While reviewing this code, please try to keep this in mind.

[fpetrogalli]

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

632

Like you have done below, it would be useful to have the following comment before the explicit cast:

// FIXME: this needs to handle scalable vectors correctly

On a side note - haven't you changed the compiler behavior here? What was resulting in a warning (taking this case for a scalable vector) would be resulting in a crash (if I understand correctly).

Given that the scalable case will look different from the fixed width one, why not just duplicate the code for each of the two case statements, making sure that the scalable one uses scalable vectors (with the current behavior) and the fixed vector one uses fixed vectors (with the warning removed). Something like:

case Type::FixedVectorTyID:
  // if the whole vector is 'undef' just reserve memory for the value.
  auto *VTy = cast<FixedVectorType>(C->getType());
  Type *ElemTy = VTy->getElementType();
  unsigned int elemNum = VTy->getNumElements();
  Result.AggregateVal.resize(elemNum);
  if (ElemTy->isIntegerTy())
    for (unsigned int i = 0; i < elemNum; ++i)
      Result.AggregateVal[i].IntVal =
          APInt(ElemTy->getPrimitiveSizeInBits(), 0);
  break;
case Type::ScalableVectorTyID:
  // if the whole vector is 'undef' just reserve memory for the value.
  auto *VTy = cast<ScalableVectorType>(C->getType());
  Type *ElemTy = VTy->getElementType();
  // FIX ME: fix implementation for scalable vectors
  unsigned int elemNum = VTy->getElementCount().Min;
  Result.AggregateVal.resize(elemNum);
  if (ElemTy->isIntegerTy())
    for (unsigned int i = 0; i < elemNum; ++i)
      Result.AggregateVal[i].IntVal =
          APInt(ElemTy->getPrimitiveSizeInBits(), 0);
  break;

[sdesmalen]

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

632

Would this not be sufficient:

case Type::ScalableVectorTyID:
  // FIXME: Add ExecutionEngine support for scalable vectors
  report_fatal_error("Scalable vector support not yet implemented in ExecutionEngine");

?

[ctetreau]

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

632

@sdesmalen reporting an error is probably the correct thing to do here.

@fpetrogalli Technically we are changing the behavior. If this branch were ever hit, then what was previously a miscompilation is now a crash. I'm working under the premise of "The LLVM project policy is that any new feature requires a test, so we must have perfect test coverage. When scalable vectors were added, the meaning of getNumElements changed. This was a behavior change that required a test. Therefore every place where the new meaning of getNumElements is used has test coverage. So if we find every usage of getNumElements, and cast the vector to FixedVectorType, every test failure that results corresponds to a place where the vector could be scalable. Every other location corresponds to a place where the vector is actually supposed to be fixed width, and the cast just makes this explicit."

Obviously, this doesn't match reality entirely due to the fact that scalable vector support isn't complete, and codepaths that should work for scalable vectors are not yet taken (and people often don't follow the rules). Due to this, I'm trying to preserve the existing structure of the code in these cases as much as possible. If I just cast to FixedVectorType at every call site of getNumElements, it avoids calling it through the base type while not modifying the control flow. That said, this code here is clearly unimplemented for scalable vectors, so I think explicitly raising an error is probably best.

[fpetrogalli]

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

632

Agree, thank you for explaining.

[ctetreau]

address code review issues

[sdesmalen]

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

1105

This needs a similar:

report_fatal_error(
            "Scalable vector support not yet implemented in ExecutionEngine");

Maybe you can also add the FIXME about scalable-vectors to the top-level comment of this file, as I suspect we want to add support for scalable vectors to this pass at some point.

[ctetreau]

address code review issues

[lhames]

LGTM, pending addressing of sdesmalen's comments.

[ctetreau]

@sdesmalen Are you satisfied with the state of this patch? May I submit this?

[sdesmalen]

In D82211#2123121, @ctetreau wrote:

@sdesmalen Are you satisfied with the state of this patch? May I submit this?

Absolutely, I didn't realise you had updated the patch. LGTM!

llvm/lib/ExecutionEngine/ExecutionEngine.cpp

case Type::StructTyID: {
           else if (ElemTy->isAggregateType()) {
               const Constant *ElemUndef = UndefValue::get(ElemTy);
               Result.AggregateVal[i] = getConstantValue(ElemUndef);
             }
           }
         }
       }
       break;
-      case Type::FixedVectorTyID:
       case Type::ScalableVectorTyID:
+        report_fatal_error(
+            "Scalable vector support not yet implemented in ExecutionEngine");
+      case Type::FixedVectorTyID:
         // if the whole vector is 'undef' just reserve memory for the value.
-        auto *VTy = cast<VectorType>(C->getType());
+        auto *VTy = cast<FixedVectorType>(C->getType());
         Type *ElemTy = VTy->getElementType();
         unsigned int elemNum = VTy->getNumElements();
         Result.AggregateVal.resize(elemNum);
         if (ElemTy->isIntegerTy())
           for (unsigned int i = 0; i < elemNum; ++i)
             Result.AggregateVal[i].IntVal =
                 APInt(ElemTy->getPrimitiveSizeInBits(), 0);
         break;
case Type::ScalableVectorTyID: {
     const ConstantDataVector *CDV = dyn_cast<ConstantDataVector>(C);
     const ConstantVector *CV = dyn_cast<ConstantVector>(C);
     const ConstantAggregateZero *CAZ = dyn_cast<ConstantAggregateZero>(C);
 
     if (CDV) {
         elemNum = CDV->getNumElements();
         ElemTy = CDV->getElementType();
     } else if (CV || CAZ) {
-        auto* VTy = cast<VectorType>(C->getType());
-        elemNum = VTy->getNumElements();
-        ElemTy = VTy->getElementType();
+      auto *VTy = cast<FixedVectorType>(C->getType());
     } else {
         llvm_unreachable("Unknown constant vector type!");
     }
 
     Result.AggregateVal.resize(elemNum);
     // Check if vector holds floats.
     if(ElemTy->isFloatTy()) {
       if (CAZ) {
case Type::X86_FP80TyID: {
     // FIXME: Will not trap if loading a signaling NaN.
     uint64_t y[2];
     memcpy(y, Ptr, 10);
     Result.IntVal = APInt(80, y);
     break;
   }
   case Type::FixedVectorTyID:
   case Type::ScalableVectorTyID: {
-    auto *VT = cast<VectorType>(Ty);
+    // FIXME: this needs to handle scalable vectors correctly
+    auto *VT = cast<FixedVectorType>(Ty);
     Type *ElemT = VT->getElementType();
     const unsigned numElems = VT->getNumElements();
     if (ElemT->isFloatTy()) {
       Result.AggregateVal.resize(numElems);
       for (unsigned i = 0; i < numElems; ++i)
         Result.AggregateVal[i].FloatVal = *((float*)Ptr+i);
     }
     if (ElemT->isDoubleTy()) {