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

[David Blaikie]

On Fri, Sep 19, 2014 at 5:29 PM, 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.
>

General idea looks good - might be easier to do the micro-review stuff in a
Phab review (some things I saw:
* DIHeaderIterator could probably just be a generic StringIterator (could
be templated on a delimiter or just hardcoded in the form of a
NullSeparatedStringIterator?)? - again, using the DI prefix for something
not in the DI* hierarchy is something I twitch about a bit... though DIRef,
etc, are precedent in favor of your naming there
* getHeaderFields could probably return a range rather than having to build
an entire vector
* getHeaderField(unsigned) could probably use std::advance
* DIHeader would go well with the StringIterator above, mabye
NullSeparatedStringBuilder? It sort of feels like a builder-y thing...
* Why the "Twinish" perfect forwarding in DIHeader::concat? Rather than
just taking a Twine?
* I guess the builder could be just a variadic function one day... *sigh*
)


>
> 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.
>
>   - I haven't bumped the "Debug Info Version".  Should I?
>

You'll have to check with Adrian about that - I think the notion was to do
that once per schema-changing LLVM open source release. Otherwise you'd
have to change it on every schema change and that involves changing every
debug info metadata test file :/ (& I'd really like us not to churn it
/that/ much if we can help it)


>
> > 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`
>   - clang with `-g`
>
> With the LTO link of opt, I'll confirm that there's a drop in memory
> usage due to fewer MDNodeOperands and look out for time regressions.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140925/9e8f0afb/attachment.html>