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

[Eric Christopher]

LGTM. Still wish metadata wasn't so heavy so we could try a string
factoring to get less duplication of values.

Thanks for all the work.

-eric

On Tue, Sep 23, 2014 at 2:15 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> 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;
>