<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 20, 2015 at 10:44 AM, 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"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Apr-17, at 19:59, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> On Fri, Apr 17, 2015 at 7:15 PM, Duncan P. N. Exon Smith<br>
> <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>>> On 2015 Apr 15, at 10:06, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>> On Tue, Apr 14, 2015 at 10:59 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>><br>
>>>> On 2015 Apr 14, at 21:46, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>><br>
>>>> On Tue, Apr 14, 2015 at 9:33 PM, Duncan P. N. Exon Smith<br>
>>>> <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>>>><br>
>>>>> `Function` definitions should support `MDNode` attachments, with a<br>
>>>>> similar syntax to instructions:<br>
>>>>><br>
>>>>> define void @foo() nounwind !attach !0 {<br>
>>>>> unreachable<br>
>>>>> }<br>
>>>>> !0 = !{}<br>
>>>>><br>
>>>>> Attachments wouldn't be allowed on declarations, just definitions.<br>
>>>>><br>
>>>>> There are two open problems this can help with:<br>
>>>>><br>
>>>>> 1. For PGO, we need somewhere to attach the function entry count.<br>
>>>>> Attaching to the function definition is a simple solution.<br>
>>>><br>
>>>> No comment - can't say I know anything about that.<br>
>>>><br>
>>>>><br>
>>>>> define void @foo() !prof !0 {<br>
>>>>> unreachable<br>
>>>>> }<br>
>>>>> !0 = !{i32 987}<br>
>>>>><br>
>>>>> 2. In debug info, we repeatedly build up a map from `Function` to the<br>
>>>>> canonical `MDSubrogram` for it.<br>
>>>><br>
>>>> Sounds great - I'd imagine this working somewhat like the way I've<br>
>>>> made implicit special members & other non-standard members of class<br>
>>>> types work in the debug info metadata, which is to say that the<br>
>>>> children reference the parent, but the parent doesn't reference the<br>
>>>> children (in this case, that would mean things like comdat folding in<br>
>>>> LTO would 'just work' - whichever function we picked would keep its<br>
>>>> debug info and attachment to its CU - the other CU would just appear<br>
>>>> not to have that function anymore - we might need a special case for<br>
>>>> comdat folding where one copy has debug info and another copy doesn't,<br>
>>>> make sure we move the metadata over if we're picking one without debug<br>
>>>> info).<br>
>>>><br>
>>>> Though that will mean that whenever we want to walk all the<br>
>>>> subprograms of a CU, we'd have to build it by walking all the<br>
>>>> Functions in a module - it might be worth checking if/when/where we<br>
>>>> "need" to do that - I suspect it's rare and maybe can be made entirely<br>
>>>> unnecessary.<br>
>>><br>
>>> I *think* we only do this once, and it's (ironically) to create the<br>
>>> "Subprogram -> CompileUnit" map.<br>
>>><br>
>>> Right, I thought that might be the case.<br>
>>><br>
>>> That highlights a problem that my proposal doesn't solve on its own.<br>
>>> While this proposal creates a "Function -> Subprogram" map, there still<br>
>>> isn't a "Subprogram -> CompileUnit" map -- that would still (for now)<br>
>>> be stored implicitly via `MDCompileUnit::getSubprograms()`.<br>
>>><br>
>>> I guess this is (also?) what I was thinking about.<br>
>>><br>
>>> Why isn't this map encoded in the `scope:` chain? Because many of the<br>
>>> `scope:` chains currently terminate at `MDFile`s or `null` instead of<br>
>>> `MDCompileUnit`s. Why? Because LTO type uniquing needs scope chains<br>
>>> to be identical.<br>
>>><br>
>>> Ah, right.<br>
>>><br>
>>> (side note: sometimes need to end in MDFile or we might need an equivalent of DILexicalBlockFile for the CU - to switch files within the same CU (things defined in headers, etc))<br>
>><br>
>> Ah, okay. I thought we could just replace them with pointers to the<br>
>> compile unit. Something like `DIFileScope` with `scope:` and<br>
>> `file:` fields would probably work? (Which means I shouldn't have<br>
>> merged the two types of "file" nodes when I introduced the new<br>
>> hierarchy. Boo.)<br>
><br>
> I'd have to double-check - maybe this is a non-issue & I've<br>
> misremembered/misunderstood things here (DILexicalBlockFile is needed<br>
> because we don't put file info on the DebugLocs to keep them small -<br>
> maybe the MDSubprograms (& MDNamespace, etc) should/could/(already<br>
> does?) just have a file attribute directly?)<br>
<br>
</div></div>Yes, they do already have a file attribute. Is that sufficient?<br></blockquote><div><br>Should be, I'd imagine.<br> </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>
>>><br>
>>> (I have a vague plan for fixing this, too: (1) move<br>
>>> ownership of Metadata to the Module (so metadata isn't leaked by<br>
>>> `lto_module_dispose()`), (2) add a "StringRef -> MDType" map to the<br>
>>> Module (only for types with an ODR-style UUID), (3) delete the concept<br>
>>> of `MDString`-based type refs and update lib/Linker to rely on the<br>
>>> "StringRef -> MDType" map in the destination Module, (4) make all<br>
>>> `scope:` chains terminate at an `MDCompileUnit` and drop "scope"-ness<br>
>>> of `MDFile`, and (5) finally drop the `subprograms:` field from<br>
>>> `MDCompileUnit`. But I'm not confident about step 4 yet.)<br>
>>><br>
>>> Sounds plausible.<br>
>>><br>
>>> (side note: any plans to do the scope-based textual IR that was thrown around during the prototype stages? It'd be another readability (& writability) improvement to not have to manually walk all the scope chains.<br>
>><br>
>> Vague plans, but yes. Doing it the way I want is blocked on<br>
>> removing type refs (maybe among other things?).<br>
>><br>
>>> Anyway, maybe after most/all the functionality improvements are made we could do a maintainability pass & see whether we could get to a practically writable/readable format... I'm still not sure how likely that is, given the fact that debug info necessarily /describes/ the source the user wrote, so it's always going to be more verbose, but maybe it's achievable)<br>
>><br>
>> Yeah, I think this is a great idea.<br>
>><br>
>>><br>
>>><br>
>>>><br>
>>>>> Keeping this mapping accurate takes<br>
>>>>> subtle logic in `lib/Linker` (see PR21910/PR22792) and it's<br>
>>>>> expensive to compute and maintain. Attaching it directly to the<br>
>>>>> `Function` designs away the problem.<br>
>>>>><br>
>>>>> define void @foo() !dbg !0 {<br>
>>>>> unreachable<br>
>>>>> }<br>
>>>>> !0 = !MDSubprogram(name: "foo", function: void ()* @foo)<br>
>>>>><br>
>>>>> Thoughts?<br>
>>>>><br>
>>>>> Moving onto implementation, I'd provide the same generic API that<br>
>>>>> `Instruction` has, and wouldn't bother with the "fast path" API for<br>
>>>>> `!dbg`. Moreover, the generic path wouldn't be slow. Since there are<br>
>>>>> fewer functions than instructions, we can afford to store the<br>
>>>>> attachments directly on the `Function` instead of off in the<br>
>>>>> `LLVMContext`.<br>
>>>>><br>
>>>>> It's not clear to me just how precious memory is in `Function`; IIRC<br>
>>>>> it's sitting at 168B right now for x86-64. IMO, a `SmallVector<..., 1>`<br>
>>>>> -- cost of 64B -- seems fine. I'll start with this if I don't hear any<br>
>>>>> objections; we can optimize later if necessary.<br>
>>>>><br>
>>>>> Otherwise, I could hack together a custom vector-like object with the<br>
>>>>> "small string" optimization.<br>
>>>><br>
>>>> Not important, but I'm missing something: what're you picturing that<br>
>>>> would be different from/better than SmallVector?<br>
>>>><br>
>>><br>
>>> Data storage would be:<br>
>>><br>
>>> struct Data {<br>
>>> struct value_type {<br>
>>> unsigned Tag;<br>
>>> TrackingMDNodeRef MD;<br>
>>> };<br>
>>><br>
>>> unsigned Capacity;<br>
>>> union {<br>
>>> unsigned LargeSize;<br>
>>> unsigned SmallTag;<br>
>>> } Unsigned;<br>
>>><br>
>>> AlignedCharArrayUnion<<br>
>>> value_type *, // LargeArray<br>
>>> TrackingMDNodeRef // SmallMD<br>
>>>> Pointer;<br>
>>> };<br>
>>> static_assert(sizeof(Data) == sizeof(void *) + sizeof(unsigned) * 2,<br>
>>> "Wrong size");<br>
>>><br>
>>> Two advantages over `SmallVector<value_type, 1>`:<br>
>>><br>
>>> - 32-bits each for size and capacity (instead of 64-bits).<br>
>>> - Domain knowledge of `value_type` allows aggressive unions.<br>
>>><br>
>>> Can't provide as much API as `std::vector<>`, but the API for metadata<br>
>>> attachments is small and opaque:<br>
>>><br>
>>> bool hasMetadata(unsigned Tag) const;<br>
>>> MDNode *getMetadata(unsigned Tag) const;<br>
>>> void setMetadata(unsigned Tag, const MDNode *MD);<br>
>>> void getAllMetadata(<br>
>>> SmallVectorImpl<std::pair<unsigned, const MDNode *>> &MDs) const;<br>
>>><br>
>>>>> Cost would be 16B per `Function`, with the<br>
>>>>> same malloc/heap costs as `SmallVector<..., 1>`. But I haven't seen a<br>
>>>>> profile that suggests this would be worth the complexity...<br>
>>>>><br>
>>>>> Any opinions?<br>
<br>
</div></div></blockquote></div><br></div></div>