[llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR

[Nicolai Hähnle via llvm-dev]

Hi Chris,

Guarding against future bugs through type-safety is a welcome
initiative, thank you!

On Mon, Mar 9, 2020 at 8:05 PM Chris Tetreault via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Proposal:
>
>                 In order to support users who only need fixed width vectors, and to ensure that nobody can accidentally call getNumElements() on a scalable vector, I am proposing the introduction of a new FixedVectorType which inherits from both VectorType and SequentialType. In turn, VectorType will no longer inherit from SequentialType. An example of what this will look like, with some misc. functions omitted for clarity:
>
> class VectorType : public Type {
> public:
>   static VectorType *get(Type *ElementType, ElementCount EC);
>   Type *getElementType() const;
>   ElementCount getElementCount() const;
>   bool isScalable() const;
> };
>
> class FixedVectorType : public VectorType, public SequentialType {
> public:
>   static FixedVectorType *get(Type *ElementType, unsigned NumElts);
> };
>
> class SequentialType : public CompositeType {
> public:
>   uint64_t getNumElements() const { return NumElements; }
> };
[snip]
> Outstanding design choice:
>
>                 One issue with this architecture as proposed is the fact that SequentialType (by way of CompositeType) inherits from Type. This introduces a diamond inheritance in FixedVectorType. Unfortunately, llvm::cast uses a c-style cast internally, so we cannot use virtual inheritance to resolve this issue. Thus, we have a few options:
[snip]
> 2. Remove CompositeType and break SequentialType’s inheritance of Type. Add functions to convert a SequentialType to and from Type. The conversion functions would work the same as those in option 1 above. Currently, there exists only one class that derives directly from CompositeType: StructType. The functionality of CompositeType can be directly moved into StructType, and APIs that use CompositeType can directly use Type and cast appropriately. We feel that this would be a fairly simple change, and we have a prototype implementation up at https://reviews.llvm.org/D75660 (Remove CompositeType class)
>
> Pros: Removing CompositeType would simplify the type hierarchy. Leaving SequentialType in would simplify some code and be more typesafe than having a getSequentialNumElements on Type.
> Cons: The value of SequentialType has also been called into question. If we wanted to remove it, now would be a good time. Conversion functions add complexity to the design. Introduces additional casting from Type.
>
> 3. Remove CompositeType and SequentialType. Roll the functions directly into the most derived classes. A helper function can be added to Type to handle choosing from FixedVectorType and ArrayType and calling getNumElements():
>
> static unsigned getSequentialNumElements() {
>   assert(isSequentialType()); // This already exists and does the
>                               // right thing
>   if (auto *AT = dyn_cast<ArrayType>(this))
>     return AT->getNumElements();
>   return cast<FixedVectorType>(this)->getNumElements();
> }
>
> A prototype implementation of this strategy can be found at https://reviews.llvm.org/D75661 (Remove SequentialType from the type heirarchy.)
>
> Pros: By removing the multiple inheritance completely, we greatly simplify the design and eliminate the need for any conversion functions. The value of CompositeType and SequentialType has been called into question, and removing them now might be of benefit to the codebase
> Cons: getSequentialNumElements() has similar issues to those that we are trying to solve in the first place and potentially subverts the whole design. Omitting getSequentialNumElements() would add lots of code duplication. Introduces additional casting from Type.

Are the issues of getSequentialNumElements() really bigger than those
of cast<SequentialType>(foo)->getNumElements()?

FWIW, the removal of CompositeType is small enough that I'm between
option 2 and 3, personally.

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.