[PATCH] D16832: Minor performance tweaks to llvm-tblgen (and a few that might be a good idea)

[David Blaikie via llvm-commits]

On Tue, Feb 2, 2016 at 5:35 PM, Alexander Riccio via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> ariccio created this revision.
> ariccio added reviewers: rnk, qcolombet, craig.topper.
> ariccio added a subscriber: llvm-commits.
>
> This patch adds a reserve call to an expensive function
> (`llvm::LoadIntrinsics`), and may fix a few other low hanging performance
> fruit (I've put them in comments for now, so we can discuss).
>
> **Motivation:**
>
> As I'm sure other developers do, when I build LLVM, I build the entire
> project with the same config (`Debug`, `MinSizeRel`, `Release`, or
> `RelWithDebInfo`). However, the `Debug` config also builds llvm-tblgen in
> `Debug` mode. Later build steps that run llvm-tblgen then can actually be
> the slowest steps in the entire build. Nobody likes slow builds.
>
>
>
> http://reviews.llvm.org/D16832
>
> Files:
>   C:/LLVM/llvm/include/llvm/TableGen/Record.h
>   C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp
>   C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp
>
> Index: C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp
> ===================================================================
> --- C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp
> +++ C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp
> @@ -49,6 +49,11 @@
>
>    unsigned MIOperandNo = 0;
>    std::set<std::string> OperandNames;
> +
> +  // TODO: can we reserve space for e elements in OperandList?
>

Seems reasonable to me - looks like we know exactly how many elements we're
going to add (there's not some (or any) complex selection criteria in the
loop that means we add fewer items than the number we iterate over.

Please just checkin the reserve call, don't bother adding the TODO.


> +  // Profiling a debug build with ETW revealed that the emplace_back
> +  // at the very bottom of this loop can take up to 227ms, largely
> +  // spent in reallocation.
>    for (unsigned i = 0, e = InDI->getNumArgs()+OutDI->getNumArgs(); i !=
> e; ++i){
>      Init *ArgInit;
>      std::string ArgName;
> Index: C:/LLVM/llvm/include/llvm/TableGen/Record.h
> ===================================================================
> --- C:/LLVM/llvm/include/llvm/TableGen/Record.h
> +++ C:/LLVM/llvm/include/llvm/TableGen/Record.h
> @@ -1307,9 +1307,12 @@
>    }
>
>    bool isSubClassOf(StringRef Name) const {
> -    for (const auto &SCPair : SuperClasses)
> +    for (const auto &SCPair : SuperClasses) {
> +      // TODO: getNameInitAsString copy constructs a new std::string,
> +      // yet we're only using it in this comparison. Is there a better
> way?
>

Skip this TODO - no, it doesn't look immediately like we can do much. I'd
probably discuss it more on-list, rather than adding a TODO just yet.


>        if (SCPair.first->getNameInitAsString() == Name)
>          return true;
> +    }
>      return false;
>    }
>
> Index: C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp
> ===================================================================
> --- C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp
> +++ C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp
> @@ -441,12 +441,24 @@
>    std::vector<Record*> I = RC.getAllDerivedDefinitions("Intrinsic");
>
>    std::vector<CodeGenIntrinsic> Result;
> +
> +  // Allocate enough memory to hold enough for worst case scenario
> +  // in the following loop. Profiling a debug build with ETW
> +  // revealed that push_back in the loop could take up to 280ms,
> +  // largely spent in reallocation.
>

Don't bother adding the comment - it seems pretty obvious why this reserve
call is here.


> +  Result.reserve(I.size());
>
>    for (unsigned i = 0, e = I.size(); i != e; ++i) {
>      bool isTarget = I[i]->getValueAsBit("isTarget");
>      if (isTarget == TargetOnly)
>        Result.push_back(CodeGenIntrinsic(I[i]));
>    }
> +
> +  // TODO: does this lambda need to accept parameters by value?
> +  // Profiling a debug build with ETW revealed that each call of
> +  // std::_Unguarded_partition (an internal function, called by
> std::sort),
> +  // can spend hundreds of milliseconds inside this lambda calling
> +  // ~CodeGenIntrinsic(), which is quite wasteful.
>

Just make the parameters const ref and don't bother adding a TODO/comment
here.


>    std::sort(Result.begin(), Result.end(),
>              [](CodeGenIntrinsic LHS, CodeGenIntrinsic RHS) {
>                return LHS.Name < RHS.Name;
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160202/26d3a0e8/attachment.html>