[llvm] c858deb - Remove asserting getters from base Type

Chris Tetreault via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 11 11:52:30 PDT 2020


I believe that the cast<>() and isa<>() calls depend on the class definition being available. This is why they are in DerivedTypes.h.

From: Eric Christopher <echristo at gmail.com>
Sent: Thursday, June 11, 2020 10:42 AM
To: Chris Tetreault <ctetreau at quicinc.com>
Cc: Eli Friedman <efriedma at quicinc.com>; llvm-commits <llvm-commits at lists.llvm.org>
Subject: [EXT] Re: [llvm] c858deb - Remove asserting getters from base Type

Hi Chris,

I think I've failed to communicate here:

I'm not suggesting changing things back. I think the fact that these are inline in one header, used in that header, and then defined in the next is limited to these two functions. I'm going to go ahead and bring them back into the current header because I think that's the best/easiest way to solve it. You seemed to have a reason for moving them, but I wanted to check in with you first.

If I'm incorrect let me know.

Thanks!

-eric

On Thu, Jun 11, 2020 at 9:35 AM Chris Tetreault <ctetreau at quicinc.com<mailto:ctetreau at quicinc.com>> wrote:
Eric,

For these two functions:

- getScalarType(): If this is broken now, then it should have been broken before. getScalarType() was previously defined in Type.h, but it called getVectorElementType() in its body, which was inline in Type.h but defined in DerivedTypes.h, so there would be an undefined reference to that.
- isVectorTy(): I changed this to call isa<VectorType>(), but this change was made in the name of maintainability (it was basically a second version of VectorType::classof()). If this is causing a real problem, I can change it back, it’s a trivial function after all.

I am strongly opposed to reinstating getVectorElementType() because it supports the bad pattern of passing everything around as a base Type *. There were countless instances of:

```
Type *VecTy = VectorType::get(…); // You named it VecTy, just assign it to a VectorType pointer!
unsigned Num = VecTy->getVectorNumElements(); // literally on the next line
```

… that I had had to fix. I personally would be fine with having getScalarType() not be inline. However, it should be possible to implement getScalarType in terms of just a Type. The element type of a VectorType is the zeroeth index of Type::ContainedTypes, so getScalarType can just return that. I can make these fixes.

All of that said, the fact is that there’s a bunch of inline functions in Type.h that are defined in DerivedTypes.h. (such as the equivalents of the asserting base type vector getters for arrays and structs) So even if I fix the issues that I added, those all need to be fixed as well. Honestly, the way these files are set up is strikes me as extremely strange. I come from the “for the most part, 1 header/source pair for 1 class” school of thought, so I think that ideally we’d have a header/source pair for each derived type, plus a TypeUtils.[h|cpp] that includes everything  all these random helpers with them all being free functions (or just, ya know, not have them).

Thanks,
   Christopher Tetreault

From: Eric Christopher <echristo at gmail.com<mailto:echristo at gmail.com>>
Sent: Wednesday, June 10, 2020 10:05 PM
To: Chris Tetreault <ctetreau at quicinc.com<mailto:ctetreau at quicinc.com>>; Christopher Tetreault <llvmlistbot at llvm.org<mailto:llvmlistbot at llvm.org>>; Eli Friedman <efriedma at quicinc.com<mailto:efriedma at quicinc.com>>
Cc: llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>
Subject: [EXT] Re: [llvm] c858deb - Remove asserting getters from base Type

Some ancient(ish) archaeology here.

This actually breaks the use of Type.h without DerivedType.h for two functions: getScalarType and isVectorTy.

You can have them used without defining them though they're marked inline. Options are: Reinstating them, moving everything that uses them also out of line, or something else I'm not thinking of right now.

Thoughts?

-eric

ps Found this with warnings in the new flang front end fwiw.

On Fri, Apr 17, 2020 at 2:04 PM Christopher Tetreault via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:

Author: Christopher Tetreault
Date: 2020-04-17T14:03:31-07:00
New Revision: c858debebc1308e748de882c745e179b1a398fa0

URL: https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0
DIFF: https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0.diff

LOG: Remove asserting getters from base Type

Summary:
Remove asserting vector getters from Type in preparation for the
VectorType refactor. The existence of these functions complicates the
refactor while adding little value.

Reviewers: dexonsmith, sdesmalen, efriedma

Reviewed By: efriedma

Subscribers: cfe-commits, hiraditya, llvm-commits

Tags: #llvm, #clang

Differential Revision: https://reviews.llvm.org/D77278

Added:


Modified:
    clang/lib/CodeGen/CGBuiltin.cpp
    llvm/include/llvm/IR/DerivedTypes.h
    llvm/include/llvm/IR/Type.h
    llvm/lib/CodeGen/CodeGenPrepare.cpp
    llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
    llvm/unittests/IR/VPIntrinsicTest.cpp

Removed:



################################################################################
diff  --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index f4832ef4afb2..8ee69740f15c 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -7573,8 +7573,7 @@ Value *CodeGenFunction::EmitSVEMaskedStore(const CallExpr *E,
   // The vector type that is stored may be
diff erent from the
   // eventual type stored to memory.
   auto VectorTy = cast<llvm::VectorType>(Ops.back()->getType());
-  auto MemoryTy =
-      llvm::VectorType::get(MemEltTy, VectorTy->getVectorElementCount());
+  auto MemoryTy = llvm::VectorType::get(MemEltTy, VectorTy->getElementCount());

   Value *Predicate = EmitSVEPredicateCast(Ops[0], MemoryTy);
   Value *BasePtr = Builder.CreateBitCast(Ops[1], MemoryTy->getPointerTo());

diff  --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index 92017448fe0d..186430754303 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -531,18 +531,6 @@ class VectorType : public Type {
   }
 };

-unsigned Type::getVectorNumElements() const {
-  return cast<VectorType>(this)->getNumElements();
-}
-
-bool Type::getVectorIsScalable() const {
-  return cast<VectorType>(this)->isScalable();
-}
-
-ElementCount Type::getVectorElementCount() const {
-  return cast<VectorType>(this)->getElementCount();
-}
-
 bool Type::isVectorTy() const { return isa<VectorType>(this); }

 /// Class to represent pointers.
@@ -597,8 +585,8 @@ Type *Type::getWithNewBitWidth(unsigned NewBitWidth) const {
       isIntOrIntVectorTy() &&
       "Original type expected to be a vector of integers or a scalar integer.");
   Type *NewType = getIntNTy(getContext(), NewBitWidth);
-  if (isVectorTy())
-    NewType = VectorType::get(NewType, getVectorElementCount());
+  if (auto *VTy = dyn_cast<VectorType>(this))
+    NewType = VectorType::get(NewType, VTy->getElementCount());
   return NewType;
 }

@@ -606,6 +594,12 @@ unsigned Type::getPointerAddressSpace() const {
   return cast<PointerType>(getScalarType())->getAddressSpace();
 }

+Type *Type::getScalarType() const {
+  if (isVectorTy())
+    return cast<VectorType>(this)->getElementType();
+  return const_cast<Type *>(this);
+}
+
 } // end namespace llvm

 #endif // LLVM_IR_DERIVEDTYPES_H

diff  --git a/llvm/include/llvm/IR/Type.h b/llvm/include/llvm/IR/Type.h
index 79d6964e3b3e..67be3ef480b7 100644
--- a/llvm/include/llvm/IR/Type.h
+++ b/llvm/include/llvm/IR/Type.h
@@ -300,11 +300,7 @@ class Type {

   /// If this is a vector type, return the element type, otherwise return
   /// 'this'.
-  Type *getScalarType() const {
-    if (isVectorTy())
-      return getVectorElementType();
-    return const_cast<Type*>(this);
-  }
+  inline Type *getScalarType() const;

   //===--------------------------------------------------------------------===//
   // Type Iteration support.
@@ -339,8 +335,8 @@ class Type {

   //===--------------------------------------------------------------------===//
   // Helper methods corresponding to subclass methods.  This forces a cast to
-  // the specified subclass and calls its accessor.  "getVectorNumElements" (for
-  // example) is shorthand for cast<VectorType>(Ty)->getNumElements().  This is
+  // the specified subclass and calls its accessor.  "getArrayNumElements" (for
+  // example) is shorthand for cast<ArrayType>(Ty)->getNumElements().  This is
   // only intended to cover the core methods that are frequently used, helper
   // methods should not be added here.

@@ -361,14 +357,6 @@ class Type {
     return ContainedTys[0];
   }

-  inline bool getVectorIsScalable() const;
-  inline unsigned getVectorNumElements() const;
-  inline ElementCount getVectorElementCount() const;
-  Type *getVectorElementType() const {
-    assert(getTypeID() == VectorTyID);
-    return ContainedTys[0];
-  }
-
   Type *getPointerElementType() const {
     assert(getTypeID() == PointerTyID);
     return ContainedTys[0];

diff  --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 5eb772d12abf..d6a216f9f12c 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -5262,7 +5262,7 @@ bool CodeGenPrepare::optimizeGatherScatterInst(Instruction *MemoryInst,
   if (!RewriteGEP && Ops.size() == 2)
     return false;

-  unsigned NumElts = Ptr->getType()->getVectorNumElements();
+  unsigned NumElts = cast<VectorType>(Ptr->getType())->getNumElements();

   IRBuilder<> Builder(MemoryInst);


diff  --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index f8c7f784bf11..a05b375d5279 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4263,7 +4263,7 @@ static bool getUniformBase(const Value *Ptr, SDValue &Base, SDValue &Index,

     Base = SDB->getValue(C);

-    unsigned NumElts = Ptr->getType()->getVectorNumElements();
+    unsigned NumElts = cast<VectorType>(Ptr->getType())->getNumElements();
     EVT VT = EVT::getVectorVT(*DAG.getContext(), TLI.getPointerTy(DL), NumElts);
     Index = DAG.getConstant(0, SDB->getCurSDLoc(), VT);
     IndexType = ISD::SIGNED_SCALED;

diff  --git a/llvm/unittests/IR/VPIntrinsicTest.cpp b/llvm/unittests/IR/VPIntrinsicTest.cpp
index 919bac4ef266..35a1f3e9b4d7 100644
--- a/llvm/unittests/IR/VPIntrinsicTest.cpp
+++ b/llvm/unittests/IR/VPIntrinsicTest.cpp
@@ -107,7 +107,7 @@ TEST_F(VPIntrinsicTest, GetParamPos) {
     if (MaskParamPos.hasValue()) {
       Type *MaskParamType = F.getArg(MaskParamPos.getValue())->getType();
       ASSERT_TRUE(MaskParamType->isVectorTy());
-      ASSERT_TRUE(MaskParamType->getVectorElementType()->isIntegerTy(1));
+      ASSERT_TRUE(cast<VectorType>(MaskParamType)->getElementType()->isIntegerTy(1));
     }

     Optional<int> VecLenParamPos =



_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200611/7bb224b3/attachment-0001.html>


More information about the llvm-commits mailing list