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

[<Alexander G. Riccio> via llvm-commits]

Alrighty. All comments addressed. I'll send a message to the mailing list
about llvm::Record::isSubClassOf(StringRef Name).

Sincerely,
Alexander Riccio
--
"Change the world or go home."
about.me/ariccio

<http://about.me/ariccio>
If left to my own devices, I will build more.
⁂

On Wed, Feb 3, 2016 at 12:15 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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/20160203/073c3ceb/attachment.html>