[PATCH] DI: Fold constant arguments into a single MDString

[Duncan P. N. Exon Smith]

> On 2014-Sep-23, at 10:18, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2014-Sep-23, at 09:52, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>>> 
>>> On Sep 22, 2014, at 5:37 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>> 
>>>> On 2014-Sep-19, at 17:29, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>>> This patch addresses the first stage of PR17891 by folding constant
>>>>> arguments together into a single MDString.  Integers are stringified and
>>>>> a `\0` character is used as a separator.
>>>> 
>>>> I've attached updated LLVM and clang patches, rebased this afternoon
>>>> with all tests in `check-all` passing.  I believe it addresses David's
>>>> comments as well.
>>>> 
>>>> There are an insane number of changes in the testcases, but changing the
>>>> schema of the dwarf tags makes that inevitable -- almost every line of
>>>> metadata had to change.  I look forward to "stage 2" of PR17891, which
>>>> can be more incremental.
>>>> 
>>>> Let me know if the updated patches look good.
>>>> 
>>>> Two caveats that came to mind while writing this:
>>>> 
>>>> - I haven't updated `docs/SourceLevelDebugging.rst`.  I'll send a
>>>> patch for that Monday-ish.
>>> 
>>> Included in the attached LLVM patch.
>>> 
>>>>> Any size measurements/test results (does this bootstrap?) on this?
>>>> 
>>>> Before committing, I'll use a release+asserts clang to build:
>>>> 
>>>> - opt with `-g -flto`
>>> 
>>> I profiled `ld64`'s link of `llvm-lto`:
>>> 
>>> - baseline: 8m21s, 16.41 GB
>>> - modified: 7m56s, 14.25 GB
>>> 
>>> The memory change is about what I expected.
>> 
>> Very nice!
>> 
>>> 
>>>> - clang with `-g`
>>> 
>>> Bootstrap was clean.
>>> 
>>> --
>>> 
>>> Rebased patches attached.
>>> 
>>> <0001-DI-Header-MDString-llvm-v2.patch><0002-DI-Header-MDString-clang-v2.patch>
>> 
>> Because of the logistics involved in updating all the testcases, it would be best to commit this after http://reviews.llvm.org/D4919.
>> 
>> I have one question about the API design: Is DIHeader used for anything besides constructing MDStrings?
> 
> Nope.
> 
>> DIHeaderIterator derefs to a StringRef instead of a DIHeader object, which appears counterintuitive to me.
> 
> Renamed to `DIHeaderFieldIterator`, and renamed the unit test macro
> to `MAKE_FIELD_ITERATOR`.
> 
>> I wonder if it would be feasible/sensible to turn DIHeader into a single variadic function template rather than a class. Or maybe DIHeader should be called something akin to DIHeaderBuilder?
> 
> It's a local class, so it doesn't need the `DI` prefix either.
> Renamed it to `HeaderBuilder`.
> 
>> All the other DI* classes are wrappers around MDNodes (and, more importantly, provide convenient accessors to the fields) and DIHeader breaks with that convention.
> 
> Right, it was more misleading than I realized.
> 
> New patches attached.  (Clang patch is unchanged.)

These patches actually compile.  One of my renames was wrong.  Here
is what changed from v3:

--- a/lib/IR/DIBuilder.cpp
+++ b/lib/IR/DIBuilder.cpp
@@ -745,7 +745,7 @@ DICompositeType DIBuilder::createVectorType(uint64_t Size, uint64_t AlignInBits,
 
 static HeaderBuilder setTypeFlagsInHeader(StringRef Header,
                                           unsigned FlagsToSet) {
-  HeaderBuilderIterator I(Header);
+  DIHeaderFieldIterator I(Header);
   std::advance(I, 6);
 
   unsigned Flags;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-DI-Header-MDString-llvm-v4.patch
Type: application/octet-stream
Size: 2147899 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140923/f97a4ca4/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-DI-Header-MDString-clang-v4.patch
Type: application/octet-stream
Size: 93100 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140923/f97a4ca4/attachment-0001.obj>