<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">I believe that the cast<>() and isa<>() calls depend on the class definition being available. This is why they are in DerivedTypes.h.
<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><b>From:</b> Eric Christopher <echristo@gmail.com> <br>
<b>Sent:</b> Thursday, June 11, 2020 10:42 AM<br>
<b>To:</b> Chris Tetreault <ctetreau@quicinc.com><br>
<b>Cc:</b> Eli Friedman <efriedma@quicinc.com>; llvm-commits <llvm-commits@lists.llvm.org><br>
<b>Subject:</b> [EXT] Re: [llvm] c858deb - Remove asserting getters from base Type<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Hi Chris,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I think I've failed to communicate here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">If I'm incorrect let me know.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">-eric<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Thu, Jun 11, 2020 at 9:35 AM Chris Tetreault <<a href="mailto:ctetreau@quicinc.com">ctetreau@quicinc.com</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Eric,<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">For these two functions:<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">- 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.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">- 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.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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:<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">```<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Type *VecTy = VectorType::get(…); // You named it VecTy, just assign it to a VectorType pointer!<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">unsigned Num = VecTy->getVectorNumElements(); // literally on the next line<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">```<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">… 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.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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).<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Thanks,<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> Christopher Tetreault<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b>From:</b> Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>>
<br>
<b>Sent:</b> Wednesday, June 10, 2020 10:05 PM<br>
<b>To:</b> Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>>; Christopher Tetreault <<a href="mailto:llvmlistbot@llvm.org" target="_blank">llvmlistbot@llvm.org</a>>; Eli Friedman <<a href="mailto:efriedma@quicinc.com" target="_blank">efriedma@quicinc.com</a>><br>
<b>Cc:</b> llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
<b>Subject:</b> [EXT] Re: [llvm] c858deb - Remove asserting getters from base Type<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Some ancient(ish) archaeology here.<o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">This actually breaks the use of Type.h without DerivedType.h for two functions: getScalarType and isVectorTy.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Thoughts?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">-eric<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">ps Found this with warnings in the new flang front end fwiw.<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">On Fri, Apr 17, 2020 at 2:04 PM Christopher Tetreault via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><br>
Author: Christopher Tetreault<br>
Date: 2020-04-17T14:03:31-07:00<br>
New Revision: c858debebc1308e748de882c745e179b1a398fa0<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0" target="_blank">
https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0.diff" target="_blank">
https://github.com/llvm/llvm-project/commit/c858debebc1308e748de882c745e179b1a398fa0.diff</a><br>
<br>
LOG: Remove asserting getters from base Type<br>
<br>
Summary:<br>
Remove asserting vector getters from Type in preparation for the<br>
VectorType refactor. The existence of these functions complicates the<br>
refactor while adding little value.<br>
<br>
Reviewers: dexonsmith, sdesmalen, efriedma<br>
<br>
Reviewed By: efriedma<br>
<br>
Subscribers: cfe-commits, hiraditya, llvm-commits<br>
<br>
Tags: #llvm, #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D77278" target="_blank">
https://reviews.llvm.org/D77278</a><br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
clang/lib/CodeGen/CGBuiltin.cpp<br>
llvm/include/llvm/IR/DerivedTypes.h<br>
llvm/include/llvm/IR/Type.h<br>
llvm/lib/CodeGen/CodeGenPrepare.cpp<br>
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
llvm/unittests/IR/VPIntrinsicTest.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp<br>
index f4832ef4afb2..8ee69740f15c 100644<br>
--- a/clang/lib/CodeGen/CGBuiltin.cpp<br>
+++ b/clang/lib/CodeGen/CGBuiltin.cpp<br>
@@ -7573,8 +7573,7 @@ Value *CodeGenFunction::EmitSVEMaskedStore(const CallExpr *E,<br>
// The vector type that is stored may be <br>
diff erent from the<br>
// eventual type stored to memory.<br>
auto VectorTy = cast<llvm::VectorType>(Ops.back()->getType());<br>
- auto MemoryTy =<br>
- llvm::VectorType::get(MemEltTy, VectorTy->getVectorElementCount());<br>
+ auto MemoryTy = llvm::VectorType::get(MemEltTy, VectorTy->getElementCount());<br>
<br>
Value *Predicate = EmitSVEPredicateCast(Ops[0], MemoryTy);<br>
Value *BasePtr = Builder.CreateBitCast(Ops[1], MemoryTy->getPointerTo());<br>
<br>
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h<br>
index 92017448fe0d..186430754303 100644<br>
--- a/llvm/include/llvm/IR/DerivedTypes.h<br>
+++ b/llvm/include/llvm/IR/DerivedTypes.h<br>
@@ -531,18 +531,6 @@ class VectorType : public Type {<br>
}<br>
};<br>
<br>
-unsigned Type::getVectorNumElements() const {<br>
- return cast<VectorType>(this)->getNumElements();<br>
-}<br>
-<br>
-bool Type::getVectorIsScalable() const {<br>
- return cast<VectorType>(this)->isScalable();<br>
-}<br>
-<br>
-ElementCount Type::getVectorElementCount() const {<br>
- return cast<VectorType>(this)->getElementCount();<br>
-}<br>
-<br>
bool Type::isVectorTy() const { return isa<VectorType>(this); }<br>
<br>
/// Class to represent pointers.<br>
@@ -597,8 +585,8 @@ Type *Type::getWithNewBitWidth(unsigned NewBitWidth) const {<br>
isIntOrIntVectorTy() &&<br>
"Original type expected to be a vector of integers or a scalar integer.");<br>
Type *NewType = getIntNTy(getContext(), NewBitWidth);<br>
- if (isVectorTy())<br>
- NewType = VectorType::get(NewType, getVectorElementCount());<br>
+ if (auto *VTy = dyn_cast<VectorType>(this))<br>
+ NewType = VectorType::get(NewType, VTy->getElementCount());<br>
return NewType;<br>
}<br>
<br>
@@ -606,6 +594,12 @@ unsigned Type::getPointerAddressSpace() const {<br>
return cast<PointerType>(getScalarType())->getAddressSpace();<br>
}<br>
<br>
+Type *Type::getScalarType() const {<br>
+ if (isVectorTy())<br>
+ return cast<VectorType>(this)->getElementType();<br>
+ return const_cast<Type *>(this);<br>
+}<br>
+<br>
} // end namespace llvm<br>
<br>
#endif // LLVM_IR_DERIVEDTYPES_H<br>
<br>
diff --git a/llvm/include/llvm/IR/Type.h b/llvm/include/llvm/IR/Type.h<br>
index 79d6964e3b3e..67be3ef480b7 100644<br>
--- a/llvm/include/llvm/IR/Type.h<br>
+++ b/llvm/include/llvm/IR/Type.h<br>
@@ -300,11 +300,7 @@ class Type {<br>
<br>
/// If this is a vector type, return the element type, otherwise return<br>
/// 'this'.<br>
- Type *getScalarType() const {<br>
- if (isVectorTy())<br>
- return getVectorElementType();<br>
- return const_cast<Type*>(this);<br>
- }<br>
+ inline Type *getScalarType() const;<br>
<br>
//===--------------------------------------------------------------------===//<br>
// Type Iteration support.<br>
@@ -339,8 +335,8 @@ class Type {<br>
<br>
//===--------------------------------------------------------------------===//<br>
// Helper methods corresponding to subclass methods. This forces a cast to<br>
- // the specified subclass and calls its accessor. "getVectorNumElements" (for<br>
- // example) is shorthand for cast<VectorType>(Ty)->getNumElements(). This is<br>
+ // the specified subclass and calls its accessor. "getArrayNumElements" (for<br>
+ // example) is shorthand for cast<ArrayType>(Ty)->getNumElements(). This is<br>
// only intended to cover the core methods that are frequently used, helper<br>
// methods should not be added here.<br>
<br>
@@ -361,14 +357,6 @@ class Type {<br>
return ContainedTys[0];<br>
}<br>
<br>
- inline bool getVectorIsScalable() const;<br>
- inline unsigned getVectorNumElements() const;<br>
- inline ElementCount getVectorElementCount() const;<br>
- Type *getVectorElementType() const {<br>
- assert(getTypeID() == VectorTyID);<br>
- return ContainedTys[0];<br>
- }<br>
-<br>
Type *getPointerElementType() const {<br>
assert(getTypeID() == PointerTyID);<br>
return ContainedTys[0];<br>
<br>
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp<br>
index 5eb772d12abf..d6a216f9f12c 100644<br>
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp<br>
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp<br>
@@ -5262,7 +5262,7 @@ bool CodeGenPrepare::optimizeGatherScatterInst(Instruction *MemoryInst,<br>
if (!RewriteGEP && Ops.size() == 2)<br>
return false;<br>
<br>
- unsigned NumElts = Ptr->getType()->getVectorNumElements();<br>
+ unsigned NumElts = cast<VectorType>(Ptr->getType())->getNumElements();<br>
<br>
IRBuilder<> Builder(MemoryInst);<br>
<br>
<br>
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
index f8c7f784bf11..a05b375d5279 100644<br>
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
@@ -4263,7 +4263,7 @@ static bool getUniformBase(const Value *Ptr, SDValue &Base, SDValue &Index,<br>
<br>
Base = SDB->getValue(C);<br>
<br>
- unsigned NumElts = Ptr->getType()->getVectorNumElements();<br>
+ unsigned NumElts = cast<VectorType>(Ptr->getType())->getNumElements();<br>
EVT VT = EVT::getVectorVT(*DAG.getContext(), TLI.getPointerTy(DL), NumElts);<br>
Index = DAG.getConstant(0, SDB->getCurSDLoc(), VT);<br>
IndexType = ISD::SIGNED_SCALED;<br>
<br>
diff --git a/llvm/unittests/IR/VPIntrinsicTest.cpp b/llvm/unittests/IR/VPIntrinsicTest.cpp<br>
index 919bac4ef266..35a1f3e9b4d7 100644<br>
--- a/llvm/unittests/IR/VPIntrinsicTest.cpp<br>
+++ b/llvm/unittests/IR/VPIntrinsicTest.cpp<br>
@@ -107,7 +107,7 @@ TEST_F(VPIntrinsicTest, GetParamPos) {<br>
if (MaskParamPos.hasValue()) {<br>
Type *MaskParamType = F.getArg(MaskParamPos.getValue())->getType();<br>
ASSERT_TRUE(MaskParamType->isVectorTy());<br>
- ASSERT_TRUE(MaskParamType->getVectorElementType()->isIntegerTy(1));<br>
+ ASSERT_TRUE(cast<VectorType>(MaskParamType)->getElementType()->isIntegerTy(1));<br>
}<br>
<br>
Optional<int> VecLenParamPos =<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</body>
</html>