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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked an inline comment as done.
ctetreau added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/ExecutionEngine.cpp:630
         // 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();
----------------
sdesmalen wrote:
> fpetrogalli wrote:
> > 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;
> > ```
> 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");```
> ?
@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82211