[llvm] [LLVM][TableGen] Minor cleanup in CGIOperandList (PR #142721)
Rahul Joshi via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 24 11:01:41 PDT 2025
https://github.com/jurahul updated https://github.com/llvm/llvm-project/pull/142721
>From 4f680ce0e2568c0cddfc973c45e783eb1cb4435f Mon Sep 17 00:00:00 2001
From: Rahul Joshi <rjoshi at nvidia.com>
Date: Tue, 3 Jun 2025 22:58:26 -0700
Subject: [PATCH 1/2] [LLVM][TableGen] Minor cleanup in CodeGenInstruction
- Use StringRef instead of std::string for `AsmString`.
- Change `hadOperandNamed` to return index as std::optional and
rename it to `findOperandNamed`.
- Change `SubOperandAlias` to return std::optional and rename it
to `findSubOperandAlias`.
---
llvm/utils/TableGen/AsmMatcherEmitter.cpp | 6 +--
llvm/utils/TableGen/CodeEmitterGen.cpp | 9 ++--
.../TableGen/Common/CodeGenInstruction.cpp | 47 ++++++++-----------
.../TableGen/Common/CodeGenInstruction.h | 15 +++---
4 files changed, 34 insertions(+), 43 deletions(-)
diff --git a/llvm/utils/TableGen/AsmMatcherEmitter.cpp b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
index b6d9c9f3a1584..7e43758c39c4d 100644
--- a/llvm/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
@@ -1712,11 +1712,11 @@ void AsmMatcherInfo::buildInstructionOperandReference(MatchableInfo *II,
MatchableInfo::AsmOperand *Op = &II->AsmOperands[AsmOpIdx];
// Map this token to an operand.
- unsigned Idx;
- if (!Operands.hasOperandNamed(OperandName, Idx))
+ std::optional<unsigned> MayBeIdx = Operands.findOperandNamed(OperandName);
+ if (!MayBeIdx)
PrintFatalError(II->TheDef->getLoc(),
"error: unable to find operand: '" + OperandName + "'");
-
+ unsigned Idx = *MayBeIdx;
// If the instruction operand has multiple suboperands, but the parser
// match class for the asm operand is still the default "ImmAsmOperand",
// then handle each suboperand separately.
diff --git a/llvm/utils/TableGen/CodeEmitterGen.cpp b/llvm/utils/TableGen/CodeEmitterGen.cpp
index 2fe40450abfda..14dffb438fcba 100644
--- a/llvm/utils/TableGen/CodeEmitterGen.cpp
+++ b/llvm/utils/TableGen/CodeEmitterGen.cpp
@@ -123,12 +123,11 @@ bool CodeEmitterGen::addCodeToMergeInOperand(const Record *R,
// operand number. Non-matching operands are assumed to be in
// order.
unsigned OpIdx;
- std::pair<unsigned, unsigned> SubOp;
- if (CGI.Operands.hasSubOperandAlias(VarName, SubOp)) {
- OpIdx = CGI.Operands[SubOp.first].MIOperandNo + SubOp.second;
- } else if (CGI.Operands.hasOperandNamed(VarName, OpIdx)) {
+ if (auto SubOp = CGI.Operands.findSubOperandAlias(VarName)) {
+ OpIdx = CGI.Operands[SubOp->first].MIOperandNo + SubOp->second;
+ } else if (auto MayBeOpIdx = CGI.Operands.findOperandNamed(VarName)) {
// Get the machine operand number for the indicated operand.
- OpIdx = CGI.Operands[OpIdx].MIOperandNo;
+ OpIdx = CGI.Operands[*MayBeOpIdx].MIOperandNo;
} else {
PrintError(R, Twine("No operand named ") + VarName + " in record " +
R->getName());
diff --git a/llvm/utils/TableGen/Common/CodeGenInstruction.cpp b/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
index 0dfcf200d7e4b..db4e1b78793d3 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
@@ -183,8 +183,8 @@ CGIOperandList::CGIOperandList(const Record *R) : TheDef(R) {
// If we have no explicit sub-op dag, but have an top-level encoder
// method, the single encoder will multiple sub-ops, itself.
OpInfo.EncoderMethodNames[0] = EncoderMethod;
- for (unsigned j = 1; j < NumOps; ++j)
- OpInfo.DoNotEncode[j] = true;
+ OpInfo.DoNotEncode.set();
+ OpInfo.DoNotEncode[0] = false;
}
MIOperandNo += NumOps;
@@ -199,36 +199,31 @@ CGIOperandList::CGIOperandList(const Record *R) : TheDef(R) {
/// specified name, abort.
///
unsigned CGIOperandList::getOperandNamed(StringRef Name) const {
- unsigned OpIdx;
- if (hasOperandNamed(Name, OpIdx))
- return OpIdx;
+ std::optional<unsigned> OpIdx = findOperandNamed(Name);
+ if (OpIdx)
+ return *OpIdx;
PrintFatalError(TheDef->getLoc(), "'" + TheDef->getName() +
"' does not have an operand named '$" +
Name + "'!");
}
-/// hasOperandNamed - Query whether the instruction has an operand of the
-/// given name. If so, return true and set OpIdx to the index of the
-/// operand. Otherwise, return false.
-bool CGIOperandList::hasOperandNamed(StringRef Name, unsigned &OpIdx) const {
+/// findOperandNamed - Query whether the instruction has an operand of the
+/// given name. If so, the index of the operand. Otherwise, return std::nullopt.
+std::optional<unsigned> CGIOperandList::findOperandNamed(StringRef Name) const {
assert(!Name.empty() && "Cannot search for operand with no name!");
- for (unsigned i = 0, e = OperandList.size(); i != e; ++i)
- if (OperandList[i].Name == Name) {
- OpIdx = i;
- return true;
- }
- return false;
+ for (const auto &[Index, Opnd] : enumerate(OperandList))
+ if (Opnd.Name == Name)
+ return Index;
+ return std::nullopt;
}
-bool CGIOperandList::hasSubOperandAlias(
- StringRef Name, std::pair<unsigned, unsigned> &SubOp) const {
+std::optional<std::pair<unsigned, unsigned>>
+CGIOperandList::findSubOperandAlias(StringRef Name) const {
assert(!Name.empty() && "Cannot search for operand with no name!");
auto SubOpIter = SubOpAliases.find(Name);
- if (SubOpIter != SubOpAliases.end()) {
- SubOp = SubOpIter->second;
- return true;
- }
- return false;
+ if (SubOpIter != SubOpAliases.end())
+ return SubOpIter->second;
+ return std::nullopt;
}
std::pair<unsigned, unsigned>
@@ -251,9 +246,7 @@ CGIOperandList::ParseOperandName(StringRef Op, bool AllowWholeOp) {
OpName = OpName.substr(0, DotIdx);
}
- unsigned OpIdx;
-
- if (std::pair<unsigned, unsigned> SubOp; hasSubOperandAlias(OpName, SubOp)) {
+ if (auto SubOp = findSubOperandAlias(OpName)) {
// Found a name for a piece of an operand, just return it directly.
if (!SubOpName.empty()) {
PrintFatalError(
@@ -262,10 +255,10 @@ CGIOperandList::ParseOperandName(StringRef Op, bool AllowWholeOp) {
": Cannot use dotted suboperand name within suboperand '" +
OpName + "'");
}
- return SubOp;
+ return *SubOp;
}
- OpIdx = getOperandNamed(OpName);
+ unsigned OpIdx = getOperandNamed(OpName);
if (SubOpName.empty()) { // If no suboperand name was specified:
// If one was needed, throw.
diff --git a/llvm/utils/TableGen/Common/CodeGenInstruction.h b/llvm/utils/TableGen/Common/CodeGenInstruction.h
index 3a5abc55319b1..c3a8d1156dda3 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstruction.h
+++ b/llvm/utils/TableGen/Common/CodeGenInstruction.h
@@ -127,10 +127,9 @@ class CGIOperandList {
/// getTiedOperand - If this operand is tied to another one, return the
/// other operand number. Otherwise, return -1.
int getTiedRegister() const {
- for (const CGIOperandList::ConstraintInfo &CI : Constraints) {
+ for (const CGIOperandList::ConstraintInfo &CI : Constraints)
if (CI.isTied())
return CI.getTiedOperand();
- }
return -1;
}
};
@@ -176,13 +175,13 @@ class CGIOperandList {
/// specified name, abort.
unsigned getOperandNamed(StringRef Name) const;
- /// hasOperandNamed - Query whether the instruction has an operand of the
- /// given name. If so, return true and set OpIdx to the index of the
- /// operand. Otherwise, return false.
- bool hasOperandNamed(StringRef Name, unsigned &OpIdx) const;
+ /// findOperandNamed - Query whether the instruction has an operand of the
+ /// given name. If so, the index of the operand. Otherwise, return
+ /// std::nullopt.
+ std::optional<unsigned> findOperandNamed(StringRef Name) const;
- bool hasSubOperandAlias(StringRef Name,
- std::pair<unsigned, unsigned> &SubOp) const;
+ std::optional<std::pair<unsigned, unsigned>>
+ findSubOperandAlias(StringRef Name) const;
/// ParseOperandName - Parse an operand name like "$foo" or "$foo.bar",
/// where $foo is a whole operand and $foo.bar refers to a suboperand.
>From b53587d3ce7f11fba38d0aee8f3b1080def99c14 Mon Sep 17 00:00:00 2001
From: Rahul Joshi <rjoshi at nvidia.com>
Date: Tue, 24 Jun 2025 11:01:12 -0700
Subject: [PATCH 2/2] Review feedback
---
llvm/utils/TableGen/AsmMatcherEmitter.cpp | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/llvm/utils/TableGen/AsmMatcherEmitter.cpp b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
index 7e43758c39c4d..f916998fc3c10 100644
--- a/llvm/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
@@ -1712,22 +1712,21 @@ void AsmMatcherInfo::buildInstructionOperandReference(MatchableInfo *II,
MatchableInfo::AsmOperand *Op = &II->AsmOperands[AsmOpIdx];
// Map this token to an operand.
- std::optional<unsigned> MayBeIdx = Operands.findOperandNamed(OperandName);
- if (!MayBeIdx)
+ std::optional<unsigned> Idx = Operands.findOperandNamed(OperandName);
+ if (!Idx)
PrintFatalError(II->TheDef->getLoc(),
"error: unable to find operand: '" + OperandName + "'");
- unsigned Idx = *MayBeIdx;
// If the instruction operand has multiple suboperands, but the parser
// match class for the asm operand is still the default "ImmAsmOperand",
// then handle each suboperand separately.
- if (Op->SubOpIdx == -1 && Operands[Idx].MINumOperands > 1) {
- const Record *Rec = Operands[Idx].Rec;
+ if (Op->SubOpIdx == -1 && Operands[*Idx].MINumOperands > 1) {
+ const Record *Rec = Operands[*Idx].Rec;
assert(Rec->isSubClassOf("Operand") && "Unexpected operand!");
const Record *MatchClass = Rec->getValueAsDef("ParserMatchClass");
if (MatchClass && MatchClass->getValueAsString("Name") == "Imm") {
// Insert remaining suboperands after AsmOpIdx in II->AsmOperands.
StringRef Token = Op->Token; // save this in case Op gets moved
- for (unsigned SI = 1, SE = Operands[Idx].MINumOperands; SI != SE; ++SI) {
+ for (unsigned SI = 1, SE = Operands[*Idx].MINumOperands; SI != SE; ++SI) {
MatchableInfo::AsmOperand NewAsmOp(/*IsIsolatedToken=*/true, Token);
NewAsmOp.SubOpIdx = SI;
II->AsmOperands.insert(II->AsmOperands.begin() + AsmOpIdx + SI,
@@ -1740,7 +1739,7 @@ void AsmMatcherInfo::buildInstructionOperandReference(MatchableInfo *II,
}
// Set up the operand class.
- Op->Class = getOperandClass(Operands[Idx], Op->SubOpIdx);
+ Op->Class = getOperandClass(Operands[*Idx], Op->SubOpIdx);
Op->OrigSrcOpName = OperandName;
// If the named operand is tied, canonicalize it to the untied operand.
@@ -1752,14 +1751,14 @@ void AsmMatcherInfo::buildInstructionOperandReference(MatchableInfo *II,
// "inc $dst"
// so that we know how to provide the $dst operand when filling in the result.
int OITied = -1;
- if (Operands[Idx].MINumOperands == 1)
- OITied = Operands[Idx].getTiedRegister();
+ if (Operands[*Idx].MINumOperands == 1)
+ OITied = Operands[*Idx].getTiedRegister();
if (OITied != -1) {
// The tied operand index is an MIOperand index, find the operand that
// contains it.
- std::pair<unsigned, unsigned> Idx = Operands.getSubOperandNumber(OITied);
- OperandName = Operands[Idx.first].Name;
- Op->SubOpIdx = Idx.second;
+ auto [OpIdx, SubopIdx] = Operands.getSubOperandNumber(OITied);
+ OperandName = Operands[OpIdx].Name;
+ Op->SubOpIdx = SubopIdx;
}
Op->SrcOpName = OperandName;
More information about the llvm-commits
mailing list