Page MenuHomePhabricator

[Perf] Simplify llvm::StringSwitch and improve incremental rebuild by 54%
ClosedPublic

Authored by davezarzycki on Feb 17 2018, 5:25 PM.

Details

Summary

Simplify llvm::StringSwitch and improve incremental rebuild performance by 54% on dual Xeon 8168 machines.

Diff Detail

Repository
rL LLVM

Event Timeline

davezarzycki created this revision.Feb 17 2018, 5:25 PM
jordan_rose added inline comments.
include/llvm/ADT/StringSwitch.h
49

This makes it impossible to return a move-only type by reference (although I suppose you can still do it by pointer).

73–75

Until StringRef gets a known-size constructor I don't think we should do this, even if it's not kicking in in most cases. It's also serving another purpose right now, which is to make sure the argument strings are compile-time constants. I can see that it would generate tons of duplicate code this way, though.

(StringRef has long been overdo for a constexpr known-size constructor anyway.)

davezarzycki added inline comments.Feb 19 2018, 9:30 AM
include/llvm/ADT/StringSwitch.h
49

Doug signed of on this in a separate and simpler review.

73–75

strlen() is effectively constexpr (I just verified this in a test file). Unless there is something nontrivial I'm missing about StringRef that would prevent strlen() from being compile away, there should be no difference in code output.

davezarzycki added inline comments.Feb 19 2018, 9:38 AM
include/llvm/ADT/StringSwitch.h
73–75

Oh, I also tried adding a known size constructor to StringRef and that blew up spectacularly. Too much code likes to create large fixed size char arrays, fill in the first few bytes, and then call StringRef(array) and trust that strlen() finds the NUL byte.

craig.topper added inline comments.
include/llvm/ADT/StringSwitch.h
73–75

There's a subclass of StringRef called StringLiteral that has a fixed size constructor. Can that be used here?

davezarzycki added inline comments.Feb 19 2018, 9:46 AM
include/llvm/ADT/StringSwitch.h
73–75

Neat-o. I spent hours yesterday trying to figure out if a mix of type traits and template magic could detect string literals inside of StringRef, but I didn't think of just subclassing StringRef and I forgot that __builtin_strlen() existed. That should work.

jordan_rose added inline comments.Feb 19 2018, 9:51 AM
include/llvm/ADT/StringSwitch.h
73–75

Oh, whoa, I didn't know that existed. Thanks, Craig! (Now to go use it in some places in Swift…)

Switched to StringLiteral and verified that the performance gain still exists.

davezarzycki marked an inline comment as done.
davezarzycki edited the summary of this revision. (Show Details)

Fixed Google unit test.

I can separate out the macro system if people want it to have separate review.

davezarzycki edited the summary of this revision. (Show Details)

Sorry for the STRING_SWITCH proposal/noise. I was working on that in parallel with the templated version fixes. Now that I'm comparing the code gen of the two, I'm impressed with the optimizer generating essentially the STRING_SWITCH code gen. I'll keep the macro version in my back pocket in case I find a scenario where the optimizer fails to optimize the templated version code gen.

jordan_rose added inline comments.Feb 20 2018, 10:33 AM
include/llvm/ADT/StringRef.h
875

I don't think this should be marked a hack. Sometimes people want to make StringRefs containing nul bytes. It's reasonable that it's explicit now, though.

Drop the 'HACK' comment from the diff.

davezarzycki added a reviewer: jordan_rose.

I think this final diff addresses all concerns, including some amount of move-only type compatibility (it compiles, but I'm not sure the experience is pleasurable).

Jordan -- Chris recently gave me commit-after-approval access. Might you please approve this? Thanks!

jordan_rose accepted this revision.Feb 26 2018, 10:15 AM

Go for it!

This revision is now accepted and ready to land.Feb 26 2018, 10:15 AM
davezarzycki closed this revision.Feb 26 2018, 10:46 AM

LLVM: r326109
clang: r326110