[LLVM][TableGen] Code cleanup in FastISelEmitter.cpp #139644
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use StringRef instead of std::string for `InstructionMemo::Name`.
- Use range for loops, zip_equal and structured bindings in loops.
- Use llvm::any_of instead of manual loops.
- Use ListSeparator.
- Remove {} around single-line if-else chains.
- Use ArrayRef<> instead of const vector reference for function args.
- Change `getLegalCName` to accept a `StringRef` to avoid
StringRef->std::string casting in several places.
- Use StringRef instead of std::string for `OpcodeName` (and in
associated maps).
Tested by verifying no changes in .inc files with and without this
change.
|
@llvm/pr-subscribers-tablegen Author: Rahul Joshi (jurahul) Changes
Tested by verifying no changes in .inc files with and without this change. Full diff: https://github.com/llvm/llvm-project/pull/139644.diff 1 Files Affected:
diff --git a/llvm/utils/TableGen/FastISelEmitter.cpp b/llvm/utils/TableGen/FastISelEmitter.cpp
index 9aa6ec1064276..e31d60fc0e42a 100644
--- a/llvm/utils/TableGen/FastISelEmitter.cpp
+++ b/llvm/utils/TableGen/FastISelEmitter.cpp
@@ -35,7 +35,7 @@ using namespace llvm;
///
namespace {
struct InstructionMemo {
- std::string Name;
+ StringRef Name;
const CodeGenRegisterClass *RC;
std::string SubRegNo;
std::vector<std::string> PhysRegs;
@@ -71,10 +71,7 @@ class ImmPredicateSet {
return Entry - 1;
}
- const TreePredicateFn &getPredicate(unsigned i) {
- assert(i < PredsByName.size());
- return PredsByName[i];
- }
+ const TreePredicateFn &getPredicate(unsigned Idx) { return PredsByName[Idx]; }
typedef std::vector<TreePredicateFn>::const_iterator iterator;
iterator begin() const { return PredsByName.begin(); }
@@ -151,37 +148,32 @@ struct OperandsSignature {
bool empty() const { return Operands.empty(); }
bool hasAnyImmediateCodes() const {
- for (unsigned i = 0, e = Operands.size(); i != e; ++i)
- if (Operands[i].isImm() && Operands[i].getImmCode() != 0)
- return true;
- return false;
+ return llvm::any_of(Operands, [](OpKind Kind) {
+ return Kind.isImm() && Kind.getImmCode() != 0;
+ });
}
/// getWithoutImmCodes - Return a copy of this with any immediate codes forced
/// to zero.
OperandsSignature getWithoutImmCodes() const {
OperandsSignature Result;
- for (unsigned i = 0, e = Operands.size(); i != e; ++i)
- if (!Operands[i].isImm())
- Result.Operands.push_back(Operands[i]);
- else
- Result.Operands.push_back(OpKind::getImm(0));
+ Result.Operands.resize(Operands.size());
+ for (auto [RO, O] : zip_equal(Result.Operands, Operands))
+ RO = O.isImm() ? OpKind::getImm(0) : O;
return Result;
}
- void emitImmediatePredicate(raw_ostream &OS, ImmPredicateSet &ImmPredicates) {
- bool EmittedAnything = false;
- for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
- if (!Operands[i].isImm())
+ void emitImmediatePredicate(raw_ostream &OS,
+ ImmPredicateSet &ImmPredicates) const {
+ ListSeparator LS(" &&\n ");
+ for (auto [Idx, Opnd] : enumerate(Operands)) {
+ if (!Opnd.isImm())
continue;
- unsigned Code = Operands[i].getImmCode();
+ unsigned Code = Opnd.getImmCode();
if (Code == 0)
continue;
- if (EmittedAnything)
- OS << " &&\n ";
-
TreePredicateFn PredFn = ImmPredicates.getPredicate(Code - 1);
// Emit the type check.
@@ -189,10 +181,9 @@ struct OperandsSignature {
ValueTypeByHwMode VVT = TP->getTree(0)->getType(0);
assert(VVT.isSimple() &&
"Cannot use variable value types with fast isel");
- OS << "VT == " << getEnumName(VVT.getSimple().SimpleTy) << " && ";
+ OS << LS << "VT == " << getEnumName(VVT.getSimple().SimpleTy) << " && ";
- OS << PredFn.getFnName() << "(imm" << i << ')';
- EmittedAnything = true;
+ OS << PredFn.getFnName() << "(imm" << Idx << ')';
}
}
@@ -304,77 +295,74 @@ struct OperandsSignature {
void PrintParameters(raw_ostream &OS) const {
ListSeparator LS;
- for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
+ for (const auto [Idx, Opnd] : enumerate(Operands)) {
OS << LS;
- if (Operands[i].isReg()) {
- OS << "Register Op" << i;
- } else if (Operands[i].isImm()) {
- OS << "uint64_t imm" << i;
- } else if (Operands[i].isFP()) {
- OS << "const ConstantFP *f" << i;
- } else {
+ if (Opnd.isReg())
+ OS << "Register Op" << Idx;
+ else if (Opnd.isImm())
+ OS << "uint64_t imm" << Idx;
+ else if (Opnd.isFP())
+ OS << "const ConstantFP *f" << Idx;
+ else
llvm_unreachable("Unknown operand kind!");
- }
}
}
- void PrintArguments(raw_ostream &OS,
- const std::vector<std::string> &PR) const {
- assert(PR.size() == Operands.size());
+ void PrintArguments(raw_ostream &OS, ArrayRef<std::string> PhyRegs) const {
ListSeparator LS;
- for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
- if (PR[i] != "")
+ for (const auto [Idx, Opnd, PhyReg] : enumerate(Operands, PhyRegs)) {
+ if (PhyReg != "") {
// Implicit physical register operand.
continue;
+ }
OS << LS;
- if (Operands[i].isReg()) {
- OS << "Op" << i;
- } else if (Operands[i].isImm()) {
- OS << "imm" << i;
- } else if (Operands[i].isFP()) {
- OS << "f" << i;
- } else {
+ if (Opnd.isReg())
+ OS << "Op" << Idx;
+ else if (Opnd.isImm())
+ OS << "imm" << Idx;
+ else if (Opnd.isFP())
+ OS << "f" << Idx;
+ else
llvm_unreachable("Unknown operand kind!");
- }
}
}
void PrintArguments(raw_ostream &OS) const {
ListSeparator LS;
- for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
+ for (const auto [Idx, Opnd] : enumerate(Operands)) {
OS << LS;
- if (Operands[i].isReg()) {
- OS << "Op" << i;
- } else if (Operands[i].isImm()) {
- OS << "imm" << i;
- } else if (Operands[i].isFP()) {
- OS << "f" << i;
- } else {
+ if (Opnd.isReg())
+ OS << "Op" << Idx;
+ else if (Opnd.isImm())
+ OS << "imm" << Idx;
+ else if (Opnd.isFP())
+ OS << "f" << Idx;
+ else
llvm_unreachable("Unknown operand kind!");
- }
}
}
- void PrintManglingSuffix(raw_ostream &OS, const std::vector<std::string> &PR,
+ void PrintManglingSuffix(raw_ostream &OS, ArrayRef<std::string> PhyRegs,
ImmPredicateSet &ImmPredicates,
bool StripImmCodes = false) const {
- for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
- if (PR[i] != "")
+ for (const auto [PR, Opnd] : zip_equal(PhyRegs, Operands)) {
+ if (PR != "") {
// Implicit physical register operand. e.g. Instruction::Mul expect to
// select to a binary op. On x86, mul may take a single operand with
// the other operand being implicit. We must emit something that looks
// like a binary instruction except for the very inner fastEmitInst_*
// call.
continue;
- Operands[i].printManglingSuffix(OS, ImmPredicates, StripImmCodes);
+ }
+ Opnd.printManglingSuffix(OS, ImmPredicates, StripImmCodes);
}
}
void PrintManglingSuffix(raw_ostream &OS, ImmPredicateSet &ImmPredicates,
bool StripImmCodes = false) const {
- for (unsigned i = 0, e = Operands.size(); i != e; ++i)
- Operands[i].printManglingSuffix(OS, ImmPredicates, StripImmCodes);
+ for (const OpKind Opnd : Operands)
+ Opnd.printManglingSuffix(OS, ImmPredicates, StripImmCodes);
}
};
} // End anonymous namespace
@@ -386,14 +374,14 @@ class FastISelMap {
typedef std::multimap<int, InstructionMemo> PredMap;
typedef std::map<MVT::SimpleValueType, PredMap> RetPredMap;
typedef std::map<MVT::SimpleValueType, RetPredMap> TypeRetPredMap;
- typedef std::map<std::string, TypeRetPredMap> OpcodeTypeRetPredMap;
+ typedef std::map<StringRef, TypeRetPredMap> OpcodeTypeRetPredMap;
typedef std::map<OperandsSignature, OpcodeTypeRetPredMap>
OperandsOpcodeTypeRetPredMap;
OperandsOpcodeTypeRetPredMap SimplePatterns;
// This is used to check that there are no duplicate predicates
- std::set<std::tuple<OperandsSignature, std::string, MVT::SimpleValueType,
+ std::set<std::tuple<OperandsSignature, StringRef, MVT::SimpleValueType,
MVT::SimpleValueType, std::string>>
SimplePatternsCheck;
@@ -412,20 +400,16 @@ class FastISelMap {
private:
void emitInstructionCode(raw_ostream &OS, const OperandsSignature &Operands,
- const PredMap &PM, const std::string &RetVTName);
+ const PredMap &PM, StringRef RetVTName);
};
} // End anonymous namespace
-static std::string getOpcodeName(const Record *Op,
- const CodeGenDAGPatterns &CGP) {
- return CGP.getSDNodeInfo(Op).getEnumName().str();
-}
-
-static std::string getLegalCName(std::string OpName) {
- std::string::size_type pos = OpName.find("::");
- if (pos != std::string::npos)
- OpName.replace(pos, 2, "_");
- return OpName;
+static std::string getLegalCName(StringRef OpName) {
+ std::string CName = OpName.str();
+ std::string::size_type Pos = CName.find("::");
+ if (Pos != std::string::npos)
+ CName.replace(Pos, 2, "_");
+ return CName;
}
FastISelMap::FastISelMap(StringRef instns) : InstNS(instns) {}
@@ -452,10 +436,7 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
const CodeGenTarget &Target = CGP.getTargetInfo();
// Scan through all the patterns and record the simple ones.
- for (CodeGenDAGPatterns::ptm_iterator I = CGP.ptm_begin(), E = CGP.ptm_end();
- I != E; ++I) {
- const PatternToMatch &Pattern = *I;
-
+ for (const PatternToMatch &Pattern : CGP.ptms()) {
// For now, just look at Instructions, so that we don't have to worry
// about emitting multiple instructions for a pattern.
TreePatternNode &Dst = Pattern.getDstPattern();
@@ -464,15 +445,15 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
const Record *Op = Dst.getOperator();
if (!Op->isSubClassOf("Instruction"))
continue;
- CodeGenInstruction &II = CGP.getTargetInfo().getInstruction(Op);
- if (II.Operands.empty())
+ CodeGenInstruction &Inst = CGP.getTargetInfo().getInstruction(Op);
+ if (Inst.Operands.empty())
continue;
// Allow instructions to be marked as unavailable for FastISel for
// certain cases, i.e. an ISA has two 'and' instruction which differ
// by what registers they can use but are otherwise identical for
// codegen purposes.
- if (II.FastISelShouldIgnore)
+ if (Inst.FastISelShouldIgnore)
continue;
// For now, ignore multi-instruction patterns.
@@ -493,7 +474,7 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
const CodeGenRegisterClass *DstRC = nullptr;
std::string SubRegNo;
if (Op->getName() != "EXTRACT_SUBREG") {
- const Record *Op0Rec = II.Operands[0].Rec;
+ const Record *Op0Rec = Inst.Operands[0].Rec;
if (Op0Rec->isSubClassOf("RegisterOperand"))
Op0Rec = Op0Rec->getValueAsDef("RegClass");
if (!Op0Rec->isSubClassOf("RegisterClass"))
@@ -524,7 +505,7 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
continue;
const Record *InstPatOp = InstPatNode.getOperator();
- std::string OpcodeName = getOpcodeName(InstPatOp, CGP);
+ StringRef OpcodeName = CGP.getSDNodeInfo(InstPatOp).getEnumName();
MVT::SimpleValueType RetVT = MVT::isVoid;
if (InstPatNode.getNumTypes())
RetVT = InstPatNode.getSimpleType(0);
@@ -591,7 +572,7 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
DstRC, std::move(SubRegNo), std::move(PhysRegInputs),
PredicateCheck);
- int complexity = Pattern.getPatternComplexity(CGP);
+ int Complexity = Pattern.getPatternComplexity(CGP);
auto inserted_simple_pattern = SimplePatternsCheck.insert(
{Operands, OpcodeName, VT, RetVT, PredicateCheck});
@@ -602,7 +583,7 @@ void FastISelMap::collectPatterns(const CodeGenDAGPatterns &CGP) {
// Note: Instructions with the same complexity will appear in the order
// that they are encountered.
- SimplePatterns[Operands][OpcodeName][VT][RetVT].emplace(complexity,
+ SimplePatterns[Operands][OpcodeName][VT][RetVT].emplace(Complexity,
std::move(Memo));
// If any of the operands were immediates with predicates on them, strip
@@ -631,16 +612,13 @@ void FastISelMap::printImmediatePredicates(raw_ostream &OS) {
void FastISelMap::emitInstructionCode(raw_ostream &OS,
const OperandsSignature &Operands,
- const PredMap &PM,
- const std::string &RetVTName) {
+ const PredMap &PM, StringRef RetVTName) {
// Emit code for each possible instruction. There may be
// multiple if there are subtarget concerns. A reverse iterator
// is used to produce the ones with highest complexity first.
bool OneHadNoPredicate = false;
- for (PredMap::const_reverse_iterator PI = PM.rbegin(), PE = PM.rend();
- PI != PE; ++PI) {
- const InstructionMemo &Memo = PI->second;
+ for (const auto &[_, Memo] : reverse(PM)) {
std::string PredicateCheck = Memo.PredicateCheck;
if (PredicateCheck.empty()) {
@@ -659,11 +637,11 @@ void FastISelMap::emitInstructionCode(raw_ostream &OS,
OS << " ";
}
- for (unsigned i = 0; i < Memo.PhysRegs.size(); ++i) {
- if (Memo.PhysRegs[i] != "")
+ for (const auto [Idx, PhyReg] : enumerate(Memo.PhysRegs)) {
+ if (PhyReg != "")
OS << " BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, "
- << "TII.get(TargetOpcode::COPY), " << Memo.PhysRegs[i]
- << ").addReg(Op" << i << ");\n";
+ << "TII.get(TargetOpcode::COPY), " << PhyReg << ").addReg(Op" << Idx
+ << ");\n";
}
OS << " return fastEmitInst_";
@@ -681,9 +659,8 @@ void FastISelMap::emitInstructionCode(raw_ostream &OS,
<< ");\n";
}
- if (!PredicateCheck.empty()) {
+ if (!PredicateCheck.empty())
OS << " }\n";
- }
}
// Return Register() if all of the possibilities had predicates but none
// were satisfied.
@@ -699,48 +676,38 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
const OperandsSignature &Operands = SimplePattern.first;
const OpcodeTypeRetPredMap &OTM = SimplePattern.second;
- for (const auto &I : OTM) {
- const std::string &Opcode = I.first;
- const TypeRetPredMap &TM = I.second;
-
+ for (const auto &[Opcode, TM] : OTM) {
OS << "// FastEmit functions for " << Opcode << ".\n";
OS << "\n";
// Emit one function for each opcode,type pair.
- for (const auto &TI : TM) {
- MVT::SimpleValueType VT = TI.first;
- const RetPredMap &RM = TI.second;
+ for (const auto &[VT, RM] : TM) {
if (RM.size() != 1) {
- for (const auto &RI : RM) {
- MVT::SimpleValueType RetVT = RI.first;
- const PredMap &PM = RI.second;
-
+ for (const auto &[RetVT, PM] : RM) {
OS << "Register fastEmit_" << getLegalCName(Opcode) << "_"
- << getLegalCName(getEnumName(VT).str()) << "_"
- << getLegalCName(getEnumName(RetVT).str()) << "_";
+ << getLegalCName(getEnumName(VT)) << "_"
+ << getLegalCName(getEnumName(RetVT)) << "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
OS << "(";
Operands.PrintParameters(OS);
OS << ") {\n";
- emitInstructionCode(OS, Operands, PM, getEnumName(RetVT).str());
+ emitInstructionCode(OS, Operands, PM, getEnumName(RetVT));
}
// Emit one function for the type that demultiplexes on return type.
OS << "Register fastEmit_" << getLegalCName(Opcode) << "_"
- << getLegalCName(getEnumName(VT).str()) << "_";
+ << getLegalCName(getEnumName(VT)) << "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
OS << "(MVT RetVT";
if (!Operands.empty())
OS << ", ";
Operands.PrintParameters(OS);
OS << ") {\nswitch (RetVT.SimpleTy) {\n";
- for (const auto &RI : RM) {
- MVT::SimpleValueType RetVT = RI.first;
+ for (const auto &[RetVT, _] : RM) {
OS << " case " << getEnumName(RetVT) << ": return fastEmit_"
- << getLegalCName(Opcode) << "_"
- << getLegalCName(getEnumName(VT).str()) << "_"
- << getLegalCName(getEnumName(RetVT).str()) << "_";
+ << getLegalCName(Opcode) << "_" << getLegalCName(getEnumName(VT))
+ << "_" << getLegalCName(getEnumName(RetVT)) << "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
OS << "(";
Operands.PrintArguments(OS);
@@ -751,7 +718,7 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
} else {
// Non-variadic return type.
OS << "Register fastEmit_" << getLegalCName(Opcode) << "_"
- << getLegalCName(getEnumName(VT).str()) << "_";
+ << getLegalCName(getEnumName(VT)) << "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
OS << "(MVT RetVT";
if (!Operands.empty())
@@ -777,9 +744,8 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
Operands.PrintParameters(OS);
OS << ") {\n";
OS << " switch (VT.SimpleTy) {\n";
- for (const auto &TI : TM) {
- MVT::SimpleValueType VT = TI.first;
- std::string TypeName = getEnumName(VT).str();
+ for (const auto &[VT, _] : TM) {
+ StringRef TypeName = getEnumName(VT);
OS << " case " << TypeName << ": return fastEmit_"
<< getLegalCName(Opcode) << "_" << getLegalCName(TypeName) << "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
@@ -825,15 +791,15 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
// Check each in order it was seen. It would be nice to have a good
// relative ordering between them, but we're not going for optimality
// here.
- for (unsigned i = 0, e = MI->second.size(); i != e; ++i) {
+ for (const OperandsSignature &Sig : MI->second) {
OS << " if (";
- MI->second[i].emitImmediatePredicate(OS, ImmediatePredicates);
+ Sig.emitImmediatePredicate(OS, ImmediatePredicates);
OS << ")\n if (Register Reg = fastEmit_";
- MI->second[i].PrintManglingSuffix(OS, ImmediatePredicates);
+ Sig.PrintManglingSuffix(OS, ImmediatePredicates);
OS << "(VT, RetVT, Opcode";
- if (!MI->second[i].empty())
+ if (!Sig.empty())
OS << ", ";
- MI->second[i].PrintArguments(OS);
+ Sig.PrintArguments(OS);
OS << "))\n return Reg;\n\n";
}
@@ -842,9 +808,7 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
}
OS << " switch (Opcode) {\n";
- for (const auto &I : OTM) {
- const std::string &Opcode = I.first;
-
+ for (const auto &[Opcode, _] : OTM) {
OS << " case " << Opcode << ": return fastEmit_" << getLegalCName(Opcode)
<< "_";
Operands.PrintManglingSuffix(OS, ImmediatePredicates);
|
wangpc-pp
reviewed
May 13, 2025
There was a problem hiding this comment.
Please do never amend commits after reviewers left comments. This makes it much more difficult to do incremental review, or (when combined with rebase) practically impossible.
Constant rebases aren't needed either. If you really need to update the branch (to resolve merge conflicts is the only valid reason), press the "Updata branch" button in GitHub UI.
Agreed. I don't think I did it for this PR, unless I am missing something. But I might have done for some earlier PRs as a part of addressing review comments. In that case I do tend to rebase on top of latest changes and then apply the additional delta to address review comments, and I can see how that can impede incremental reviews. I'll be more mindful going forward and refrain from doing that. |
No, but I thought I should warn you before your do :)
Thanks a lot! |
jurahul
force-pushed
the
fastisel_use_stringref
branch
from
dc5965c to
c28c0ef
Compare
|
I pushed the delta into a new commit and did not rebase, hopefully that helps review just the new delta |
s-barannikov
approved these changes
May 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
InstructionMemo::Name.getLegalCNameto accept aStringRefto avoid StringRef->std::string casting in several places.OpcodeName(and in associated maps).Tested by verifying no changes in .inc files with and without this change.