[PATCH] MC: Use StringRef to avoid strcmp/strlen on non-null-term'd strings.

[Benjamin Kramer]

On 13.10.2013, at 22:02, Will Dietz <w at wdtz.org> wrote:

> On Sun, Oct 13, 2013 at 12:46 PM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>>> Are either concerns something to be sufficiently concerned about that
>>>> they should be measured, and if so any recommenedations on how I might
>>>> do so?
>>> 
>>> I would prefer not doing this.
>>> 
>>> * StringRef in a compile-time initialized array adds static initializers because the struct is no longer POD.
>> 
>> I don't think this code is used in static initializers. I got the same
>> size for .init and .init_array when building clang with and without
>> this patch in debug mode.
>> 
> 
> Diving into this further, a dump of the resulting llc binaries shows
> without my patch
> the X86ProcSchedKV (for example) table is present, but with my patch the section
> is all zeros.
> 
> I'm also seeing identical .init/.init_array sizes, but further poking shows
> that symbols like
> _GLOBAL__sub_I_ARMMCTargetDesc.cpp
> are significantly larger.  Presumably this is where the initialization is done,
> and setting a breakpoint on it confirms its called before main().
> 
> Darn, I was hopeful the compiler was able to see through the StringRef
> constructor and statically evaluate the calls to strlen() etc :).
> Would be pretty nifty, although wouldn't solve issue of the tables being larger.

LLVM's optimizer can often do that. Tables like this generate a lot of sequential loads and stores though and globalopt isn't smart enough to fold them all away :(

There's a long term goal of removing all static initializers from LLVM and turning on a warning to keep them from creeping back in.

> Anyway, attached is an updated patch that avoids these issues
> (as well as the dodgy format() code) by using a new comparator
> for doing the lower_bound with a StringRef, as suggested.
> 
> Look good?

Much better, LGTM! One thing to watch out for is MSVC, it's infamous for failing when comparators with different types are used and demands more overloads. Probably best to just commit and see if the buildbots like it.

- Ben