[llvm] c858deb - Remove asserting getters from base Type
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 11 10:42:15 PDT 2020
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>
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>
> *Sent:* Wednesday, June 10, 2020 10:05 PM
> *To:* Chris Tetreault <ctetreau at quicinc.com>; Christopher Tetreault <
> llvmlistbot at llvm.org>; Eli Friedman <efriedma at quicinc.com>
> *Cc:* llvm-commits <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> 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
> 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/531a1473/attachment.html>
More information about the llvm-commits
mailing list