This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Cleanup prior to introdution of FixedVectorType
AbandonedPublic

Authored by ctetreau on Apr 1 2020, 12:28 PM.

Details

Reviewers
efriedma
aartbik
Summary

Remove asserting getters from base Type. The existence of these
functions make the VectorTyes refactor more difficult while adding
little value.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 1 2020, 12:28 PM
ctetreau updated this revision to Diff 254279.Apr 1 2020, 12:55 PM

run clang-format

ctetreau updated this revision to Diff 254285.Apr 1 2020, 1:06 PM

make lints work

My thoughts, in no particular order:

  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.
  2. This needs to be split up; it's way too long to read, and it's mixing functional and non-functional changes.
  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.
  4. Some of the changes are leading to really crazy indentation because the cast<> is more characters.
  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.

  1. 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

  1. 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.

  1. 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.

ctetreau updated this revision to Diff 254327.Apr 1 2020, 3:43 PM

Split commit up into multiple smaller commits:

1 commit per llvm library, 1 for clang, and 1 for mlir. Plus one final commit to remove the functions from Type

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 3:43 PM
ctetreau updated this revision to Diff 254331.Apr 1 2020, 3:57 PM

attempt to stack all commits

ctetreau abandoned this revision.Apr 8 2020, 3:44 PM