From a91c34118294efbf08ebd11eed96fce83bf35f3c Mon Sep 17 00:00:00 2001 From: Jim Stichnoth Date: Tue, 5 Apr 2016 15:31:43 -0700 Subject: [PATCH] Subzero: Improve non-MINIMAL string performance. In a DUMP-enabled build, such as the standard Release+Asserts build, translator performance has regressed as a result of 467ffe51bebcb3ae3e3ce745c38fc20e8837c31c (https://codereview.chromium.org/1838753002). This is because Variable and CfgNode names are being instantiated unconditionally, rather than on-demand. This CL restores most of that performance by going back to being on-demand. Note that it should have no effect on MINIMAL build performance. Also, it turns out that Variable::getName() does not really need the Cfg* parameter, so that is removed (and all its callers are fixed transitively). In addition, Variable and CfgNode are made more uniform with respect to each other in terms of inline definitions of the ctor, getName(), and setName(). BUG= none R=jpp@chromium.org, kschimpf@google.com Review URL: https://codereview.chromium.org/1866463002 . --- src/IceCfg.cpp | 2 +- src/IceCfgNode.cpp | 17 ++++------------- src/IceCfgNode.h | 13 ++++++++++--- src/IceInst.cpp | 2 +- src/IceOperand.cpp | 10 ++-------- src/IceOperand.h | 25 +++++++++++-------------- src/IceRegAlloc.cpp | 10 +++++----- src/IceTargetLoweringARM32.cpp | 4 ++-- src/IceTargetLoweringMIPS32.cpp | 4 ++-- src/IceTargetLoweringX86BaseImpl.h | 8 ++++---- 10 files changed, 42 insertions(+), 53 deletions(-) diff --git a/src/IceCfg.cpp b/src/IceCfg.cpp index 33a59924c..4f316c128 100644 --- a/src/IceCfg.cpp +++ b/src/IceCfg.cpp @@ -1062,7 +1062,7 @@ void Cfg::emit() { if (getFlags().getDecorateAsm()) { for (Variable *Var : getVariables()) { if (Var->getStackOffset() && !Var->isRematerializable()) { - Str << "\t" << Var->getSymbolicStackOffset(this) << " = " + Str << "\t" << Var->getSymbolicStackOffset() << " = " << Var->getStackOffset() << "\n"; } } diff --git a/src/IceCfgNode.cpp b/src/IceCfgNode.cpp index 4b5268a72..94acff980 100644 --- a/src/IceCfgNode.cpp +++ b/src/IceCfgNode.cpp @@ -26,15 +26,6 @@ namespace Ice { -CfgNode::CfgNode(Cfg *Func, SizeT Number) : Func(Func), Number(Number) { - if (BuildDefs::dump()) { - Name = - NodeString::createWithString(Func, "__" + std::to_string(getIndex())); - } else { - Name = NodeString::createWithoutString(Func); - } -} - // Adds an instruction to either the Phi list or the regular instruction list. // Validates that all Phis are added before all regular instructions. void CfgNode::appendInst(Inst *Instr, bool AllowPhisAnywhere) { @@ -797,7 +788,7 @@ bool CfgNode::livenessValidateIntervals(Liveness *Liveness) const { auto Next = Start + 1; Str << "Duplicate LR begin, block " << getName() << ", instructions " << Start->second << " & " << Next->second << ", variable " - << Liveness->getVariable(Start->first, this)->getName(Func) << "\n"; + << Liveness->getVariable(Start->first, this)->getName() << "\n"; } for (auto Start = MapEnd.begin(); (Start = std::adjacent_find(Start, MapEnd.end(), ComparePair)) != @@ -806,7 +797,7 @@ bool CfgNode::livenessValidateIntervals(Liveness *Liveness) const { auto Next = Start + 1; Str << "Duplicate LR end, block " << getName() << ", instructions " << Start->second << " & " << Next->second << ", variable " - << Liveness->getVariable(Start->first, this)->getName(Func) << "\n"; + << Liveness->getVariable(Start->first, this)->getName() << "\n"; } } @@ -1402,7 +1393,7 @@ void CfgNode::dump(Cfg *Func) const { for (SizeT i = 0; i < LiveIn.size(); ++i) { if (LiveIn[i]) { Variable *Var = Liveness->getVariable(i, this); - Str << " %" << Var->getName(Func); + Str << " %" << Var->getName(); if (Func->isVerbose(IceV_RegOrigins) && Var->hasReg()) { Str << ":" << Func->getTarget()->getRegName(Var->getRegNum(), @@ -1428,7 +1419,7 @@ void CfgNode::dump(Cfg *Func) const { for (SizeT i = 0; i < LiveOut.size(); ++i) { if (LiveOut[i]) { Variable *Var = Liveness->getVariable(i, this); - Str << " %" << Var->getName(Func); + Str << " %" << Var->getName(); if (Func->isVerbose(IceV_RegOrigins) && Var->hasReg()) { Str << ":" << Func->getTarget()->getRegName(Var->getRegNum(), diff --git a/src/IceCfgNode.h b/src/IceCfgNode.h index cf3e83c98..6d7811c35 100644 --- a/src/IceCfgNode.h +++ b/src/IceCfgNode.h @@ -37,7 +37,11 @@ public: /// Access the label number and name for this node. SizeT getIndex() const { return Number; } void resetIndex(SizeT NewNumber) { Number = NewNumber; } - NodeString getName() const { return Name; } + std::string getName() const { + if (Name.hasStdString()) + return Name.toString(); + return "__" + std::to_string(NumberOrig); + } void setName(const std::string &NewName) { if (NewName.empty()) return; @@ -118,10 +122,13 @@ public: } private: - CfgNode(Cfg *Func, SizeT Number); + CfgNode(Cfg *Func, SizeT Number) + : Func(Func), Number(Number), NumberOrig(Number), + Name(NodeString::createWithoutString(Func)) {} bool livenessValidateIntervals(Liveness *Liveness) const; Cfg *const Func; - SizeT Number; /// invariant: Func->Nodes[Number]==this + SizeT Number; /// invariant: Func->Nodes[Number]==this + const SizeT NumberOrig; /// used for name auto-generation NodeString Name; SizeT LoopNestDepth = 0; /// the loop nest depth of this node bool HasReturn = false; /// does this block need an epilog? diff --git a/src/IceInst.cpp b/src/IceInst.cpp index 6cd2a0470..a30a6327a 100644 --- a/src/IceInst.cpp +++ b/src/IceInst.cpp @@ -452,7 +452,7 @@ Inst *InstPhi::lower(Cfg *Func) { assert(Dest); Variable *NewSrc = Func->makeVariable(Dest->getType()); if (BuildDefs::dump()) - NewSrc->setName(Func, Dest->getName(Func) + "_phi"); + NewSrc->setName(Func, Dest->getName() + "_phi"); if (auto *NewSrc64On32 = llvm::dyn_cast(NewSrc)) NewSrc64On32->initHiLo(Func); this->Dest = NewSrc; diff --git a/src/IceOperand.cpp b/src/IceOperand.cpp index 462ba9a61..50d70b935 100644 --- a/src/IceOperand.cpp +++ b/src/IceOperand.cpp @@ -191,12 +191,6 @@ void LiveRange::trim(InstNumberT Lower) { ++TrimmedBegin; } -std::string Variable::getName(const Cfg *Func) const { - if (Func == nullptr) - return "__" + std::to_string(getIndex()); - return Name.toString(); -} - const Variable *Variable::asType(const Cfg *Func, Type Ty, RegNumT NewRegNum) const { // Note: This returns a Variable, even if the "this" object is a subclass of @@ -539,12 +533,12 @@ void Variable::dump(const Cfg *Func, Ostream &Str) const { if (!BuildDefs::dump()) return; if (Func == nullptr) { - Str << "%" << getName(Func); + Str << "%" << getName(); return; } if (Func->isVerbose(IceV_RegOrigins) || (!hasReg() && !Func->getTarget()->hasComputedFrame())) - Str << "%" << getName(Func); + Str << "%" << getName(); if (hasReg()) { if (Func->isVerbose(IceV_RegOrigins)) Str << ":"; diff --git a/src/IceOperand.h b/src/IceOperand.h index 04895f61b..06b11e69c 100644 --- a/src/IceOperand.h +++ b/src/IceOperand.h @@ -647,9 +647,12 @@ public: } SizeT getIndex() const { return Number; } - std::string getName(const Cfg *Func) const; + std::string getName() const { + if (Name.hasStdString()) + return Name.toString(); + return "__" + std::to_string(getIndex()); + } virtual void setName(const Cfg *Func, const std::string &NewName) { - (void)Func; if (NewName.empty()) return; Name = VariableString::createWithString(Func, NewName); @@ -669,10 +672,10 @@ public: void setStackOffset(int32_t Offset) { StackOffset = Offset; } /// Returns the variable's stack offset in symbolic form, to improve /// readability in DecorateAsm mode. - std::string getSymbolicStackOffset(const Cfg *Func) const { + std::string getSymbolicStackOffset() const { if (!BuildDefs::dump()) return ""; - return "lv$" + getName(Func); + return "lv$" + getName(); } bool hasReg() const { return getRegNum().hasValue(); } @@ -755,12 +758,6 @@ protected: Vars = VarsReal; Vars[0] = this; NumVars = 1; - if (BuildDefs::dump()) { - Name = VariableString::createWithString( - Func, "__" + std::to_string(getIndex())); - } else { - Name = VariableString::createWithoutString(Func); - } } /// Number is unique across all variables, and is used as a (bit)vector index /// for liveness analysis. @@ -805,8 +802,8 @@ public: void setName(const Cfg *Func, const std::string &NewName) override { Variable::setName(Func, NewName); if (LoVar && HiVar) { - LoVar->setName(Func, getName(Func) + "__lo"); - HiVar->setName(Func, getName(Func) + "__hi"); + LoVar->setName(Func, getName() + "__lo"); + HiVar->setName(Func, getName() + "__hi"); } } @@ -835,8 +832,8 @@ public: LoVar->setIsArg(getIsArg()); HiVar->setIsArg(getIsArg()); if (BuildDefs::dump()) { - LoVar->setName(Func, getName(Func) + "__lo"); - HiVar->setName(Func, getName(Func) + "__hi"); + LoVar->setName(Func, getName() + "__lo"); + HiVar->setName(Func, getName() + "__hi"); } } diff --git a/src/IceRegAlloc.cpp b/src/IceRegAlloc.cpp index 2eb3a844b..e7538525c 100644 --- a/src/IceRegAlloc.cpp +++ b/src/IceRegAlloc.cpp @@ -169,12 +169,12 @@ bool LinearScan::livenessValidateIntervals( for (SizeT VarNum : DefsWithoutUses) { Variable *Var = Vars[VarNum]; Str << "LR def without use, instruction " << LRBegin[VarNum] - << ", variable " << Var->getName(Func) << "\n"; + << ", variable " << Var->getName() << "\n"; } for (SizeT VarNum : UsesBeforeDefs) { Variable *Var = Vars[VarNum]; Str << "LR use before def, instruction " << LREnd[VarNum] << ", variable " - << Var->getName(Func) << "\n"; + << Var->getName() << "\n"; } return false; } @@ -277,12 +277,12 @@ void LinearScan::initForInfOnly() { for (SizeT VarNum : DefsWithoutUses) { Variable *Var = Vars[VarNum]; Str << "LR def without use, instruction " << LRBegin[VarNum] - << ", variable " << Var->getName(Func) << "\n"; + << ", variable " << Var->getName() << "\n"; } for (SizeT VarNum : UsesBeforeDefs) { Variable *Var = Vars[VarNum]; Str << "LR use before def, instruction " << LREnd[VarNum] - << ", variable " << Var->getName(Func) << "\n"; + << ", variable " << Var->getName() << "\n"; } } llvm::report_fatal_error("initForInfOnly: Liveness error"); @@ -715,7 +715,7 @@ void LinearScan::handleNoFreeRegisters(IterationState &Iter) { Func->setError("Unable to find a physical register for an " "infinite-weight live range " "(consider using -reg-reserve): " + - Iter.Cur->getName(Func)); + Iter.Cur->getName()); Handled.push_back(Iter.Cur); return; } diff --git a/src/IceTargetLoweringARM32.cpp b/src/IceTargetLoweringARM32.cpp index 0f626e6da..54a0cfaf9 100644 --- a/src/IceTargetLoweringARM32.cpp +++ b/src/IceTargetLoweringARM32.cpp @@ -1270,7 +1270,7 @@ void TargetARM32::emitVariable(const Variable *Var) const { return; } if (Var->mustHaveReg()) { - llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName(Func) + + llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName() + ") has no register assigned - function " + Func->getFunctionName()); } @@ -1402,7 +1402,7 @@ void TargetARM32::lowerArguments() { Variable *RegisterArg = Func->makeVariable(Ty); if (BuildDefs::dump()) { - RegisterArg->setName(Func, "home_reg:" + Arg->getName(Func)); + RegisterArg->setName(Func, "home_reg:" + Arg->getName()); } RegisterArg->setIsArg(); Arg->setIsArg(false); diff --git a/src/IceTargetLoweringMIPS32.cpp b/src/IceTargetLoweringMIPS32.cpp index de8afbfd8..8b12f8879 100644 --- a/src/IceTargetLoweringMIPS32.cpp +++ b/src/IceTargetLoweringMIPS32.cpp @@ -434,7 +434,7 @@ void TargetMIPS32::lowerArguments() { Variable *RegisterArg = Func->makeVariable(Ty); auto *RegisterArg64On32 = llvm::cast(RegisterArg); if (BuildDefs::dump()) - RegisterArg64On32->setName(Func, "home_reg:" + Arg->getName(Func)); + RegisterArg64On32->setName(Func, "home_reg:" + Arg->getName()); RegisterArg64On32->initHiLo(Func); RegisterArg64On32->setIsArg(); RegisterArg64On32->getLo()->setRegNum(RegLo); @@ -451,7 +451,7 @@ void TargetMIPS32::lowerArguments() { ++NumGPRRegsUsed; Variable *RegisterArg = Func->makeVariable(Ty); if (BuildDefs::dump()) { - RegisterArg->setName(Func, "home_reg:" + Arg->getName(Func)); + RegisterArg->setName(Func, "home_reg:" + Arg->getName()); } RegisterArg->setRegNum(RegNum); RegisterArg->setIsArg(); diff --git a/src/IceTargetLoweringX86BaseImpl.h b/src/IceTargetLoweringX86BaseImpl.h index c1a5f749b..0bf03ebf3 100644 --- a/src/IceTargetLoweringX86BaseImpl.h +++ b/src/IceTargetLoweringX86BaseImpl.h @@ -886,7 +886,7 @@ void TargetX86Base::emitVariable(const Variable *Var) const { return; } if (Var->mustHaveReg()) { - llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName(Func) + + llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName() + ") has no register assigned - function " + Func->getFunctionName()); } @@ -901,7 +901,7 @@ void TargetX86Base::emitVariable(const Variable *Var) const { // Only print Offset when it is nonzero, regardless of DecorateAsm. if (Offset) { if (DecorateAsm) { - Str << Var->getSymbolicStackOffset(Func); + Str << Var->getSymbolicStackOffset(); } else { Str << Offset; } @@ -916,7 +916,7 @@ TargetX86Base::stackVarToAsmOperand(const Variable *Var) const { if (Var->hasReg()) llvm::report_fatal_error("Stack Variable has a register assigned"); if (Var->mustHaveReg()) { - llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName(Func) + + llvm::report_fatal_error("Infinite-weight Variable (" + Var->getName() + ") has no register assigned - function " + Func->getFunctionName()); } @@ -1509,7 +1509,7 @@ void TargetX86Base::lowerArguments() { // an instruction in the prolog to copy the home register to the assigned // location of Arg. if (BuildDefs::dump()) - RegisterArg->setName(Func, "home_reg:" + Arg->getName(Func)); + RegisterArg->setName(Func, "home_reg:" + Arg->getName()); RegisterArg->setRegNum(RegNum); RegisterArg->setIsArg(); Arg->setIsArg(false); -- 2.11.0