<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 17, 2016 at 3:09 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2016-Apr-17, at 15:08, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Sat, Apr 16, 2016 at 7:30 PM, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: dexonsmith<br>
> Date: Sat Apr 16 21:30:20 2016<br>
> New Revision: 266548<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=266548&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=266548&view=rev</a><br>
> Log:<br>
> IR: Use ODR to unique DICompositeType members<br>
><br>
> Merge members that are describing the same member of the same ODR type,<br>
> even if other bits differ. If the file or line differ, we don't care;<br>
> if anything else differs, it's an ODR violation (and we still don't<br>
> really care).<br>
><br>
> For DISubprogram declarations, this looks at the LinkageName and Scope.<br></span></blockquote><div><br></div><div>What we put linkage names on is a bit haphazard... I think I mentioned this in a recent review for Paul. I included some patches to Clang that might fix those - I'd suggest you check them out or I can go & find them. (and/or test this deduplication with some function-local types (in inline functions) and a type in an anonymous namespace)<br><br>Why look at the scope as well? What does that add/change?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> For DW_TAG_member instances of DIDerivedType, this looks at the Name and<br>
> Scope. In both cases, we know that the Scope follows ODR rules if it<br>
> has a non-empty identifier.<br>
><br>
> Not quite following this paragraph ^ why does this need to look at subprograms and members, rather than just the 'identifier' of the DICompositeType itself?<br>
<br>
</span>It's the subprograms and members that are being uniqued.<br></blockquote><div><br></div><div>*reads the commit message again* Right.<br><br>*ponders*<br><br>If it's any help, I think it should be fine to never touch/merge the "members" list in a DICompositeType (perhaps you're already doing this) - if a member is in the member list, just take the equivalent (linkage name, I guess).<br><br>But I guess you've got to deduplicate the things that aren't in the member list too anyway (& the order of things, and their simple names aren't enough to identify them, etc) so there's not much point doing something special for the things in the member list, perhaps.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> Added:<br>
> llvm/trunk/test/Assembler/dicompositetype-members.ll<br>
> Modified:<br>
> llvm/trunk/docs/LangRef.rst<br>
> llvm/trunk/lib/IR/LLVMContextImpl.h<br>
> llvm/trunk/test/Linker/type-unique-odr-a.ll<br>
><br>
> Modified: llvm/trunk/docs/LangRef.rst<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/LangRef.rst?rev=266548&r1=266547&r2=266548&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/LangRef.rst?rev=266548&r1=266547&r2=266548&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/docs/LangRef.rst (original)<br>
> +++ llvm/trunk/docs/LangRef.rst Sat Apr 16 21:30:20 2016<br>
> @@ -3976,7 +3976,10 @@ The following ``tag:`` values are valid:<br>
><br>
> ``DW_TAG_member`` is used to define a member of a :ref:`composite type<br>
> <DICompositeType>`. The type of the member is the ``baseType:``. The<br>
> -``offset:`` is the member's bit offset.<br>
> +``offset:`` is the member's bit offset. If the composite type has a non-empty<br>
> +``identifier:``, then it respects ODR rules. In that case, the ``scope:``<br>
> +reference will be a :ref:`metadata string <metadata-string>`, and the member<br>
> +will be uniqued solely based on its ``name:`` and ``scope:``.<br>
><br>
> ``DW_TAG_inheritance`` and ``DW_TAG_friend`` are used in the ``elements:``<br>
> field of :ref:`composite types <DICompositeType>` to describe parents and<br>
> @@ -4125,6 +4128,12 @@ metadata. The ``variables:`` field point<br>
> that must be retained, even if their IR counterparts are optimized out of<br>
> the IR. The ``type:`` field must point at an :ref:`DISubroutineType`.<br>
><br>
> +When ``isDefinition: false``, subprograms describe a declaration in the type<br>
> +tree as opposed to a definition of a funciton. If the scope is a<br>
> +:ref:`metadata string <metadata-string>` then the composite type follows ODR<br>
> +rules, and the subprogram declaration is uniqued based only on its<br>
> +``linkageName:`` and ``scope:``.<br>
> +<br>
> .. code-block:: llvm<br>
><br>
> define void @_Z3foov() !dbg !0 {<br>
> @@ -4133,7 +4142,7 @@ the IR. The ``type:`` field must point a<br>
><br>
> !0 = distinct !DISubprogram(name: "foo", linkageName: "_Zfoov", scope: !1,<br>
> file: !2, line: 7, type: !3, isLocal: true,<br>
> - isDefinition: false, scopeLine: 8,<br>
> + isDefinition: true, scopeLine: 8,<br>
> containingType: !4,<br>
> virtuality: DW_VIRTUALITY_pure_virtual,<br>
> virtualIndex: 10, flags: DIFlagPrototyped,<br>
><br>
> Modified: llvm/trunk/lib/IR/LLVMContextImpl.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.h?rev=266548&r1=266547&r2=266548&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.h?rev=266548&r1=266547&r2=266548&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/IR/LLVMContextImpl.h (original)<br>
> +++ llvm/trunk/lib/IR/LLVMContextImpl.h Sat Apr 16 21:30:20 2016<br>
> @@ -32,6 +32,7 @@<br>
> #include "llvm/IR/LLVMContext.h"<br>
> #include "llvm/IR/Metadata.h"<br>
> #include "llvm/IR/ValueHandle.h"<br>
> +#include "llvm/Support/Dwarf.h"<br>
> #include <vector><br>
><br>
> namespace llvm {<br>
> @@ -376,6 +377,12 @@ template <> struct MDNodeKeyImpl<DIDeriv<br>
> ExtraData == RHS->getRawExtraData();<br>
> }<br>
> unsigned getHashValue() const {<br>
> + // If this is a member inside an ODR type, only hash the type and the name.<br>
> + // Otherwise the hash will be stronger than<br>
> + // MDNodeSubsetEqualImpl::isODRMember().<br>
> + if (Tag == dwarf::DW_TAG_member && Name && Scope && isa<MDString>(Scope))<br>
> + return hash_combine(Name, Scope);<br>
> +<br>
> // Intentionally computes the hash on a subset of the operands for<br>
> // performance reason. The subset has to be significant enough to avoid<br>
> // collision "most of the time". There is no correctness issue in case of<br>
> @@ -384,6 +391,30 @@ template <> struct MDNodeKeyImpl<DIDeriv<br>
> }<br>
> };<br>
><br>
> +template <> struct MDNodeSubsetEqualImpl<DIDerivedType> {<br>
> + typedef MDNodeKeyImpl<DIDerivedType> KeyTy;<br>
> + static bool isSubsetEqual(const KeyTy &LHS, const DIDerivedType *RHS) {<br>
> + return isODRMember(LHS.Tag, LHS.Scope, LHS.Name, RHS);<br>
> + }<br>
> + static bool isSubsetEqual(const DIDerivedType *LHS, const DIDerivedType *RHS) {<br>
> + return isODRMember(LHS->getTag(), LHS->getRawScope(), LHS->getRawName(),<br>
> + RHS);<br>
> + }<br>
> +<br>
> + /// Subprograms compare equal if they declare the same function in an ODR<br>
> + /// type.<br>
> + static bool isODRMember(unsigned Tag, const Metadata *Scope,<br>
> + const MDString *Name, const DIDerivedType *RHS) {<br>
> + // Check whether the LHS is eligible.<br>
> + if (Tag != dwarf::DW_TAG_member || !Name || !Scope || !isa<MDString>(Scope))<br>
> + return false;<br>
> +<br>
> + // Compare to the RHS.<br>
> + return Tag == RHS->getTag() && Name == RHS->getRawName() &&<br>
> + Scope == RHS->getRawScope();<br>
> + }<br>
> +};<br>
> +<br>
> template <> struct MDNodeKeyImpl<DICompositeType> {<br>
> unsigned Tag;<br>
> MDString *Name;<br>
> @@ -537,6 +568,12 @@ template <> struct MDNodeKeyImpl<DISubpr<br>
> Variables == RHS->getRawVariables();<br>
> }<br>
> unsigned getHashValue() const {<br>
> + // If this is a declaration inside an ODR type, only hash the type and the<br>
> + // name. Otherwise the hash will be stronger than<br>
> + // MDNodeSubsetEqualImpl::isDeclarationOfODRMember().<br>
> + if (!IsDefinition && LinkageName && Scope && isa<MDString>(Scope))<br>
> + return hash_combine(LinkageName, Scope);<br>
> +<br>
> // Intentionally computes the hash on a subset of the operands for<br>
> // performance reason. The subset has to be significant enough to avoid<br>
> // collision "most of the time". There is no correctness issue in case of<br>
> @@ -545,6 +582,33 @@ template <> struct MDNodeKeyImpl<DISubpr<br>
> }<br>
> };<br>
><br>
> +template <> struct MDNodeSubsetEqualImpl<DISubprogram> {<br>
> + typedef MDNodeKeyImpl<DISubprogram> KeyTy;<br>
> + static bool isSubsetEqual(const KeyTy &LHS, const DISubprogram *RHS) {<br>
> + return isDeclarationOfODRMember(LHS.IsDefinition, LHS.Scope,<br>
> + LHS.LinkageName, RHS);<br>
> + }<br>
> + static bool isSubsetEqual(const DISubprogram *LHS, const DISubprogram *RHS) {<br>
> + return isDeclarationOfODRMember(LHS->isDefinition(), LHS->getRawScope(),<br>
> + LHS->getRawLinkageName(), RHS);<br>
> + }<br>
> +<br>
> + /// Subprograms compare equal if they declare the same function in an ODR<br>
> + /// type.<br>
> + static bool isDeclarationOfODRMember(bool IsDefinition, const Metadata *Scope,<br>
> + const MDString *LinkageName,<br>
> + const DISubprogram *RHS) {<br>
> + // Check whether the LHS is eligible.<br>
> + if (IsDefinition || !Scope || !LinkageName || !Scope ||<br>
> + !isa<MDString>(Scope))<br>
> + return false;<br>
> +<br>
> + // Compare to the RHS.<br>
> + return IsDefinition == RHS->isDefinition() && Scope == RHS->getRawScope() &&<br>
> + LinkageName == RHS->getRawLinkageName();<br>
> + }<br>
> +};<br>
> +<br>
> template <> struct MDNodeKeyImpl<DILexicalBlock> {<br>
> Metadata *Scope;<br>
> Metadata *File;<br>
><br>
> Added: llvm/trunk/test/Assembler/dicompositetype-members.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Assembler/dicompositetype-members.ll?rev=266548&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Assembler/dicompositetype-members.ll?rev=266548&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Assembler/dicompositetype-members.ll (added)<br>
> +++ llvm/trunk/test/Assembler/dicompositetype-members.ll Sat Apr 16 21:30:20 2016<br>
> @@ -0,0 +1,54 @@<br>
> +; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s<br>
> +; RUN: verify-uselistorder %s<br>
> +<br>
> +; Anchor the order of the nodes.<br>
> +!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17}<br>
> +<br>
> +; Some basic building blocks.<br>
> +; CHECK: !0 = !DIBasicType<br>
> +; CHECK-NEXT: !1 = !DIFile<br>
> +; CHECK-NEXT: !2 = !DIFile<br>
> +!0 = !DIBasicType(tag: DW_TAG_base_type, name: "name", size: 1, align: 2, encoding: DW_ATE_unsigned_char)<br>
> +!1 = !DIFile(filename: "path/to/file", directory: "/path/to/dir")<br>
> +!2 = !DIFile(filename: "path/to/other", directory: "/path/to/dir")<br>
> +<br>
> +; Define an identified type with fields and functions.<br>
> +; CHECK-NEXT: !3 = !DICompositeType(tag: DW_TAG_structure_type, name: "has-uuid",<br>
> +; CHECK-NEXT: !4 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !"has-uuid", file: !1<br>
> +; CHECK-NEXT: !5 = !DIDerivedType(tag: DW_TAG_member, name: "field2", scope: !"has-uuid", file: !1<br>
> +; CHECK-NEXT: !6 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !"has-uuid", file: !1<br>
> +; CHECK-NEXT: !7 = !DISubprogram(name: "foo", linkageName: "foo2", scope: !"has-uuid", file: !1<br>
> +!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "has-uuid", file: !1, line: 2, size: 64, align: 32, identifier: "uuid")<br>
> +!4 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !"has-uuid", file: !1, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!5 = !DIDerivedType(tag: DW_TAG_member, name: "field2", scope: !"has-uuid", file: !1, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!6 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !"has-uuid", file: !1, isDefinition: false)<br>
> +!7 = !DISubprogram(name: "foo", linkageName: "foo2", scope: !"has-uuid", file: !1, isDefinition: false)<br>
> +<br>
> +; Define an un-identified type with fields and functions.<br>
> +; CHECK-NEXT: !8 = !DICompositeType(tag: DW_TAG_structure_type, name: "no-uuid", file: !1<br>
> +; CHECK-NEXT: !9 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !8, file: !1<br>
> +; CHECK-NEXT: !10 = !DIDerivedType(tag: DW_TAG_member, name: "field2", scope: !8, file: !1<br>
> +; CHECK-NEXT: !11 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !8, file: !1<br>
> +; CHECK-NEXT: !12 = !DISubprogram(name: "foo", linkageName: "foo2", scope: !8, file: !1<br>
> +!8 = !DICompositeType(tag: DW_TAG_structure_type, name: "no-uuid", file: !1, line: 2, size: 64, align: 32)<br>
> +!9 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !8, file: !1, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!10 = !DIDerivedType(tag: DW_TAG_member, name: "field2", scope: !8, file: !1, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!11 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !8, file: !1, isDefinition: false)<br>
> +!12 = !DISubprogram(name: "foo", linkageName: "foo2", scope: !8, file: !1, isDefinition: false)<br>
> +<br>
> +; Add duplicate fields and members of "no-uuid" in a different file. These<br>
> +; should stick around, since "no-uuid" does not have an "identifier:" field.<br>
> +; CHECK-NEXT: !13 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !8, file: !2,<br>
> +; CHECK-NEXT: !14 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !8, file: !2,<br>
> +!13 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !8, file: !2, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!14 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !8, file: !2, isDefinition: false)<br>
> +<br>
> +; Add duplicate fields and members of "has-uuid" in a different file. These<br>
> +; should be merged.<br>
> +!15 = !DIDerivedType(tag: DW_TAG_member, name: "field1", scope: !"has-uuid", file: !2, line: 4, baseType: !0, size: 32, align: 32, offset: 32)<br>
> +!16 = !DISubprogram(name: "foo", linkageName: "foo1", scope: !"has-uuid", file: !2, isDefinition: false)<br>
> +<br>
> +; CHECK-NEXT: !15 = !{!4, !6}<br>
> +; CHECK-NOT: !DIDerivedType<br>
> +; CHECK-NOT: !DISubprogram<br>
> +!17 = !{!15, !16}<br>
><br>
> Modified: llvm/trunk/test/Linker/type-unique-odr-a.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/type-unique-odr-a.ll?rev=266548&r1=266547&r2=266548&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/type-unique-odr-a.ll?rev=266548&r1=266547&r2=266548&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Linker/type-unique-odr-a.ll (original)<br>
> +++ llvm/trunk/test/Linker/type-unique-odr-a.ll Sat Apr 16 21:30:20 2016<br>
> @@ -4,6 +4,10 @@<br>
> ; RUN: | %llc_dwarf -dwarf-linkage-names=Enable -filetype=obj -O0 \<br>
> ; RUN: | llvm-dwarfdump -debug-dump=info - \<br>
> ; RUN: | FileCheck %s<br>
> +; RUN: llvm-link %p/type-unique-odr-b.ll %s -S -o - \<br>
> +; RUN: | %llc_dwarf -dwarf-linkage-names=Enable -filetype=obj -O0 \<br>
> +; RUN: | llvm-dwarfdump -debug-dump=info - \<br>
> +; RUN: | FileCheck %s<br>
> ;<br>
> ; Test ODR-based type uniquing for C++ class members.<br>
> ; rdar://problem/15851313.<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>