[PATCH] D77236: [SVE] Cleanup prior to introdution of FixedVectorType

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau added a comment.

> 1. This is orthogonal to splitting VectorType, as far as I can tell.  `Ty->getVectorNumElements()` works equally well whether the implementation asserts it's a VectorType that isn't scalable, or asserts it's a FixedVectorType.

While it's not strictly neccesary, the refactor will be more straightforward with this change. Before, we would have:

  unsigned n = t->getVectorNumElements();

After the VectorTypes refactor, if t is not a FixedLengthVector, then this code will still compile but fail at runtime. However with:

  unsigned n = cast<VectorType>(t)->getNumElements();

... it will be a compile time failure.

Realistically, it would need to be at least renamed to getFixedVectorNumElements anyways to avoid confusion.

> 2. This needs to be split up; it's way too long to read, and it's mixing functional and non-functional changes.

I can split it up into a few chunks that make the corrections a bit at a time, then a final change to get rid of the functions

> 3. It might make sense to add helpers to remove the use of cast<> in more places. For example, the inputs of a shufflevector are guaranteed to be vectors.

It seems to me that we use base Type for basically everything when we could use concrete subclasses. As you can see in my patch, I fixed a few, but ideally functions would just return a VectorType if they have a VectorType. The issue is pervasive, and is enabled by the existence of these functions. This is a deep rabbit hole that I don't really want to go down in this patch.

> 4. Some of the changes are leading to really crazy indentation because the `cast<>` is more characters.

While I agree, there's not much we can do about that. Really, the problem is that we have these crazy complicated if statements that check 15 things at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77236