Page MenuHomePhabricator

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

Authored by ctetreau on Jun 19 2020, 10:19 AM.

Diff Detail

Event Timeline

ctetreau created this revision.Jun 19 2020, 10:19 AM
Herald added a project: Restricted Project. · View Herald Transcript

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 added inline comments.Jun 22 2020, 1:19 PM
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
634

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 added inline comments.Jun 23 2020, 8:05 AM
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
634

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 marked an inline comment as done.Jun 23 2020, 10:17 AM
ctetreau added inline comments.
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
634

@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 added inline comments.Jun 23 2020, 8:22 PM
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
634

Agree, thank you for explaining.

ctetreau updated this revision to Diff 273151.Jun 24 2020, 1:53 PM

address code review issues

sdesmalen added inline comments.Jun 26 2020, 7:03 AM
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
1107–1111

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 updated this revision to Diff 274220.Jun 29 2020, 1:21 PM

address code review issues

lhames accepted this revision.Jun 29 2020, 5:01 PM

LGTM, pending addressing of sdesmalen's comments.

This revision is now accepted and ready to land.Jun 29 2020, 5:01 PM
ctetreau marked an inline comment as done.Jun 30 2020, 9:02 AM

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

sdesmalen accepted this revision.Jun 30 2020, 9:18 AM

@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!

This revision was automatically updated to reflect the committed changes.