This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

ctetreau created this revision.Jun 19 2020, 10:33 AM
ctetreau updated this revision to Diff 272134.Jun 19 2020, 10:36 AM

fix permissions

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.

david-arm added inline comments.Jun 25 2020, 1:00 AM

Hi @ctetreau, could you not just cast Shuffles[0]->getType() to a FixedVectorType here, then avoid having to make the two typecasts below? That way if it is indeed a scalable vector it will blow up earlier, perhaps in a more obvious place?

ctetreau updated this revision to Diff 274213.Jun 29 2020, 12:56 PM

address code review issues

ctetreau marked an inline comment as done.Jun 29 2020, 12:58 PM
ctetreau added inline comments.

This would probably be better. I moved the cast down a few lines to bypass the early return and getNumInterleavedAccesses that allegedly works on the base vector type. This gets rid of quite a few casts.

david-arm accepted this revision.Jun 30 2020, 6:39 AM
This revision is now accepted and ready to land.Jun 30 2020, 6:39 AM
This revision was automatically updated to reflect the committed changes.