<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 28, 2014 at 4:19 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Mon, Jul 28, 2014 at 1:01 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Jul 28, 2014 at 12:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Jul 28, 2014 at 11:52 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>> > Author: mren<br>
>> > Date: Mon Jul 28 13:52:30 2014<br>
>> > New Revision: 214111<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=214111&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=214111&view=rev</a><br>
>> > Log:<br>
>> > [Debug Info] replace DIUnspecifiedParameter with DITrivialType.<br>
>> ><br>
>> > This is the first of a series of patches to handle type uniqueing of the<br>
>> > type array for a subroutine type.<br>
>> ><br>
>> > This commit makes sure unspecified_parameter is a DIType to enable<br>
>> > converting<br>
>> > the type array for a subroutine type to an array of DITypes.<br>
>><br>
>> Can you remind me (& it'll be helpful to have it in this thread for<br>
>> later reference) why unspecified type couldn't be just a plain DIType<br>
>> (or DIBasicType, etc) and needed it's own derived class?<br>
><br>
><br>
> Yes, it is mainly for size reasons. But you are right, we will only have one<br>
> unspecified_type and one unspecified_parameter for each module.<br>
> The other reason is for clarity so we know these two are trivial types<br>
> without size, offset etc.<br>
><br>
> I am okay with removing DITrivialType if it is a burden to have one more<br>
> class in our DIType hierarchy.<br>
<br>
</div>It really is a bit awkward to derive here - since unspecified type has<br>
strictly /less/ functionality (hence all the assertions that were<br>
added) - it should be further up the inheritance hierarchy, if<br>
anywhere. And it's not /exactly/ a type, though it's convenient to<br>
treat it as one.<br></blockquote><div><br></div><div>Totally agree. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
We could be hackish in a different way, and treat null in the<br>
subroutine type list as being "unspecified type" (null in the first<br>
slot means "void return", but null in later (and only trailing)<br>
positions in the array could be "unspecified parameters")<br>
<br>
The "right" thing would be for there to be a type from which DIType<br>
would derive (call it, say, DIThingsInSubroutineTypes) that we either<br>
used directly, or had a separate derived type for<br>
DIUnspecifiedParameters - that type (DIThingsInSubroutineTypes)<br>
wouldn't have any of the member functions DIType has, because it<br>
doesn't have any data/features.<br>
<br>
But then we'd need DIThingsInSubroutineTypesRef as well, etc, etc...<br>
<br>
I'm sort of inclined to use null here. Eric - what do you reckon?<br>
<br>
{null} -> "void()"<br>
{null, null} -> "void(...)"<br>
{null, <metadata node for int>, null} -> "void(int, ...)"<br>
{null, null, <metadata node for int>} -> invalid<br>
<br>
Eric - what do you reckon?<br>
<br>
Easy to implement, just have createUnspecifiedParameter return null,<br>
and have the two places that check for isUnspecifiedParameter check<br>
for null instead.<br>
<br>
Any other reasons we'd have null in a parameter type list?<br></blockquote><div><br></div><div>Sounds good to me. I can't think of any reason to have a null parameter type. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But, yeah, alternatively we just use a basic DIType and don't worry<br>
about the rest of the fields - it's a bit trickier I think (having a<br>
non-null DIType object with a bunch of operations that are invalid -<br>
null/non-nullness is a bit more of a clearer distinction - either<br>
you've got all the operations or none of them).<br></blockquote><div><br></div><div>Just like unspecified_type, we can use zero or null for fields of DIBasicType that are not needed.</div><div><br></div><div>Thanks,</div>
<div>Manman</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Thanks,<br>
> Manman<br>
><br>
>><br>
>><br>
>> I'm guessing it's for size reasons, though I don't think it's too<br>
>> important to make unspecified_type particularly compact - we'll only<br>
>> ever need one of them anyway, so we just pay the cost once per module.<br>
>> We could still have the same assertions in DIType for usability,<br>
>> though (but they're probably not necessary either - many of DIType's<br>
>> members aren't valid/meaningful for various types).<br>
>><br>
>> ><br>
>> > This commit should have no functionality change. With this commit, we<br>
>> > may<br>
>> > change unspecified type to be a DITrivialType instead of a DIType.<br>
>> ><br>
>> > Modified:<br>
>> > llvm/trunk/include/llvm/IR/DIBuilder.h<br>
>> > llvm/trunk/include/llvm/IR/DebugInfo.h<br>
>> > llvm/trunk/lib/IR/DIBuilder.cpp<br>
>> > llvm/trunk/lib/IR/DebugInfo.cpp<br>
>> ><br>
>> > Modified: llvm/trunk/include/llvm/IR/DIBuilder.h<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=214111&r1=214110&r2=214111&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=214111&r1=214110&r2=214111&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)<br>
>> > +++ llvm/trunk/include/llvm/IR/DIBuilder.h Mon Jul 28 13:52:30 2014<br>
>> > @@ -463,9 +463,9 @@ namespace llvm {<br>
>> > /// through debug info anchors.<br>
>> > void retainType(DIType T);<br>
>> ><br>
>> > - /// createUnspecifiedParameter - Create unspecified type descriptor<br>
>> > + /// createUnspecifiedParameter - Create unspecified parameter type<br>
>> > /// for a subroutine type.<br>
>> > - DIDescriptor createUnspecifiedParameter();<br>
>> > + DITrivialType createUnspecifiedParameter();<br>
>> ><br>
>> > /// getOrCreateArray - Get a DIArray, create one if required.<br>
>> > DIArray getOrCreateArray(ArrayRef<Value *> Elements);<br>
>> ><br>
>> > Modified: llvm/trunk/include/llvm/IR/DebugInfo.h<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=214111&r1=214110&r2=214111&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=214111&r1=214110&r2=214111&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)<br>
>> > +++ llvm/trunk/include/llvm/IR/DebugInfo.h Mon Jul 28 13:52:30 2014<br>
>> > @@ -128,6 +128,7 @@ public:<br>
>> > bool isDerivedType() const;<br>
>> > bool isCompositeType() const;<br>
>> > bool isBasicType() const;<br>
>> > + bool isTrivialType() const;<br>
>> > bool isVariable() const;<br>
>> > bool isSubprogram() const;<br>
>> > bool isGlobalVariable() const;<br>
>> > @@ -302,15 +303,36 @@ public:<br>
>> > /// Verify - Verify that a type descriptor is well formed.<br>
>> > bool Verify() const;<br>
>> ><br>
>> > - DIScopeRef getContext() const { return getFieldAs<DIScopeRef>(2); }<br>
>> > - StringRef getName() const { return getStringField(3); }<br>
>> > - unsigned getLineNumber() const { return getUnsignedField(4); }<br>
>> > - uint64_t getSizeInBits() const { return getUInt64Field(5); }<br>
>> > - uint64_t getAlignInBits() const { return getUInt64Field(6); }<br>
>> > + DIScopeRef getContext() const {<br>
>> > + assert(!isTrivialType() && "no context for DITrivialType");<br>
>> > + return getFieldAs<DIScopeRef>(2);<br>
>> > + }<br>
>> > + StringRef getName() const {<br>
>> > + assert(!isTrivialType() && "no name for DITrivialType");<br>
>> > + return getStringField(3);<br>
>> > + }<br>
>> > + unsigned getLineNumber() const {<br>
>> > + assert(!isTrivialType() && "no line number for DITrivialType");<br>
>> > + return getUnsignedField(4);<br>
>> > + }<br>
>> > + uint64_t getSizeInBits() const {<br>
>> > + assert(!isTrivialType() && "no SizeInBits for DITrivialType");<br>
>> > + return getUInt64Field(5);<br>
>> > + }<br>
>> > + uint64_t getAlignInBits() const {<br>
>> > + assert(!isTrivialType() && "no AlignInBits for DITrivialType");<br>
>> > + return getUInt64Field(6);<br>
>> > + }<br>
>> > // FIXME: Offset is only used for DW_TAG_member nodes. Making every<br>
>> > type<br>
>> > // carry this is just plain insane.<br>
>> > - uint64_t getOffsetInBits() const { return getUInt64Field(7); }<br>
>> > - unsigned getFlags() const { return getUnsignedField(8); }<br>
>> > + uint64_t getOffsetInBits() const {<br>
>> > + assert(!isTrivialType() && "no OffsetInBits for DITrivialType");<br>
>> > + return getUInt64Field(7);<br>
>> > + }<br>
>> > + unsigned getFlags() const {<br>
>> > + assert(!isTrivialType() && "no flag for DITrivialType");<br>
>> > + return getUnsignedField(8);<br>
>> > + }<br>
>> > bool isPrivate() const { return (getFlags() & FlagPrivate) != 0; }<br>
>> > bool isProtected() const { return (getFlags() & FlagProtected) != 0;<br>
>> > }<br>
>> > bool isForwardDecl() const { return (getFlags() & FlagFwdDecl) != 0;<br>
>> > }<br>
>> > @@ -343,6 +365,12 @@ public:<br>
>> > void replaceAllUsesWith(MDNode *D);<br>
>> > };<br>
>> ><br>
>> > +class DITrivialType : public DIType {<br>
>> > +public:<br>
>> > + explicit DITrivialType(const MDNode *N = nullptr) : DIType(N) {}<br>
>> > + bool Verify() const;<br>
>> > +};<br>
>> > +<br>
>> > /// DIBasicType - A basic type, like 'int' or 'float'.<br>
>> > class DIBasicType : public DIType {<br>
>> > public:<br>
>> > @@ -571,14 +599,6 @@ public:<br>
>> > bool Verify() const;<br>
>> > };<br>
>> ><br>
>> > -/// DIUnspecifiedParameter - This is a wrapper for unspecified<br>
>> > parameters.<br>
>> > -class DIUnspecifiedParameter : public DIDescriptor {<br>
>> > -public:<br>
>> > - explicit DIUnspecifiedParameter(const MDNode *N = nullptr)<br>
>> > - : DIDescriptor(N) {}<br>
>> > - bool Verify() const;<br>
>> > -};<br>
>> > -<br>
>> > /// DITemplateTypeParameter - This is a wrapper for template type<br>
>> > parameter.<br>
>> > class DITemplateTypeParameter : public DIDescriptor {<br>
>> > public:<br>
>> ><br>
>> > Modified: llvm/trunk/lib/IR/DIBuilder.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=214111&r1=214110&r2=214111&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=214111&r1=214110&r2=214111&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/lib/IR/DIBuilder.cpp (original)<br>
>> > +++ llvm/trunk/lib/IR/DIBuilder.cpp Mon Jul 28 13:52:30 2014<br>
>> > @@ -875,11 +875,11 @@ void DIBuilder::retainType(DIType T) {<br>
>> ><br>
>> > /// createUnspecifiedParameter - Create unspeicified type descriptor<br>
>> > /// for the subroutine type.<br>
>> > -DIDescriptor DIBuilder::createUnspecifiedParameter() {<br>
>> > +DITrivialType DIBuilder::createUnspecifiedParameter() {<br>
>> > Value *Elts[] = {<br>
>> > GetTagConstant(VMContext, dwarf::DW_TAG_unspecified_parameters)<br>
>> > };<br>
>> > - return DIDescriptor(MDNode::get(VMContext, Elts));<br>
>> > + return DITrivialType(MDNode::get(VMContext, Elts));<br>
>> > }<br>
>> ><br>
>> > /// createForwardDecl - Create a temporary forward-declared type that<br>
>> ><br>
>> > Modified: llvm/trunk/lib/IR/DebugInfo.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=214111&r1=214110&r2=214111&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=214111&r1=214110&r2=214111&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/lib/IR/DebugInfo.cpp (original)<br>
>> > +++ llvm/trunk/lib/IR/DebugInfo.cpp Mon Jul 28 13:52:30 2014<br>
>> > @@ -39,6 +39,7 @@ bool DIDescriptor::Verify() const {<br>
>> > return DbgNode &&<br>
>> > (DIDerivedType(DbgNode).Verify() ||<br>
>> > DICompositeType(DbgNode).Verify() ||<br>
>> > DIBasicType(DbgNode).Verify() ||<br>
>> > + DITrivialType(DbgNode).Verify() ||<br>
>> > DIVariable(DbgNode).Verify() ||<br>
>> > DISubprogram(DbgNode).Verify() ||<br>
>> > DIGlobalVariable(DbgNode).Verify() ||<br>
>> > DIFile(DbgNode).Verify() ||<br>
>> > DICompileUnit(DbgNode).Verify() ||<br>
>> > DINameSpace(DbgNode).Verify() ||<br>
>> > @@ -46,7 +47,6 @@ bool DIDescriptor::Verify() const {<br>
>> > DILexicalBlockFile(DbgNode).Verify() ||<br>
>> > DISubrange(DbgNode).Verify() ||<br>
>> > DIEnumerator(DbgNode).Verify() ||<br>
>> > DIObjCProperty(DbgNode).Verify() ||<br>
>> > - DIUnspecifiedParameter(DbgNode).Verify() ||<br>
>> > DITemplateTypeParameter(DbgNode).Verify() ||<br>
>> > DITemplateValueParameter(DbgNode).Verify() ||<br>
>> > DIImportedEntity(DbgNode).Verify());<br>
>> > @@ -155,6 +155,10 @@ MDNode *DIVariable::getInlinedAt() const<br>
>> > // Predicates<br>
>> ><br>
>> > //===----------------------------------------------------------------------===//<br>
>> ><br>
>> > +bool DIDescriptor::isTrivialType() const {<br>
>> > + return DbgNode && getTag() == dwarf::DW_TAG_unspecified_parameters;<br>
>> > +}<br>
>> > +<br>
>> > /// isBasicType - Return true if the specified tag is legal for<br>
>> > /// DIBasicType.<br>
>> > bool DIDescriptor::isBasicType() const {<br>
>> > @@ -225,7 +229,8 @@ bool DIDescriptor::isVariable() const {<br>
>> ><br>
>> > /// isType - Return true if the specified tag is legal for DIType.<br>
>> > bool DIDescriptor::isType() const {<br>
>> > - return isBasicType() || isCompositeType() || isDerivedType();<br>
>> > + return isBasicType() || isCompositeType() || isDerivedType() ||<br>
>> > + isTrivialType();<br>
>> > }<br>
>> ><br>
>> > /// isSubprogram - Return true if the specified tag is legal for<br>
>> > @@ -456,7 +461,7 @@ bool DIType::Verify() const {<br>
>> ><br>
>> > // FIXME: Sink this into the various subclass verifies.<br>
>> > uint16_t Tag = getTag();<br>
>> > - if (!isBasicType() && Tag != dwarf::DW_TAG_const_type &&<br>
>> > + if (!isBasicType() && !isTrivialType() && Tag !=<br>
>> > dwarf::DW_TAG_const_type &&<br>
>> > Tag != dwarf::DW_TAG_volatile_type && Tag !=<br>
>> > dwarf::DW_TAG_pointer_type &&<br>
>> > Tag != dwarf::DW_TAG_ptr_to_member_type &&<br>
>> > Tag != dwarf::DW_TAG_reference_type &&<br>
>> > @@ -471,6 +476,8 @@ bool DIType::Verify() const {<br>
>> > // a CompositeType.<br>
>> > if (isBasicType())<br>
>> > return DIBasicType(DbgNode).Verify();<br>
>> > + else if (isTrivialType())<br>
>> > + return DITrivialType(DbgNode).Verify();<br>
>> > else if (isCompositeType())<br>
>> > return DICompositeType(DbgNode).Verify();<br>
>> > else if (isDerivedType())<br>
>> > @@ -484,6 +491,10 @@ bool DIBasicType::Verify() const {<br>
>> > return isBasicType() && DbgNode->getNumOperands() == 10;<br>
>> > }<br>
>> ><br>
>> > +bool DITrivialType::Verify() const {<br>
>> > + return isTrivialType() && DbgNode->getNumOperands() == 1;<br>
>> > +}<br>
>> > +<br>
>> > /// Verify - Verify that a derived type descriptor is well formed.<br>
>> > bool DIDerivedType::Verify() const {<br>
>> > // Make sure DerivedFrom @ field 9 is TypeRef.<br>
>> > @@ -624,11 +635,6 @@ bool DILexicalBlockFile::Verify() const<br>
>> > return isLexicalBlockFile() && DbgNode->getNumOperands() == 3;<br>
>> > }<br>
>> ><br>
>> > -/// \brief Verify that an unspecified parameter descriptor is well<br>
>> > formed.<br>
>> > -bool DIUnspecifiedParameter::Verify() const {<br>
>> > - return isUnspecifiedParameter() && DbgNode->getNumOperands() == 1;<br>
>> > -}<br>
>> > -<br>
>> > /// \brief Verify that the template type parameter descriptor is well<br>
>> > formed.<br>
>> > bool DITemplateTypeParameter::Verify() const {<br>
>> > return isTemplateTypeParameter() && DbgNode->getNumOperands() == 7;<br>
>> > @@ -1290,7 +1296,7 @@ void DIEnumerator::printInternal(raw_ost<br>
>> > }<br>
>> ><br>
>> > void DIType::printInternal(raw_ostream &OS) const {<br>
>> > - if (!DbgNode)<br>
>> > + if (!DbgNode || isTrivialType())<br>
>> > return;<br>
>> ><br>
>> > StringRef Res = getName();<br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>