[TableGen][RISCV] Support sub-operands in CompressInstEmitter.cpp. #133039
Conversation
This file contains 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
I'm looking into using sub-operands for memory operands. This would use MIOperandInfo to create a single operand that contains a register and immediate as sub-operands. We can treat this as a single operand for parsing and matching in the assembler. I believe this will provide some simplifications like removing the InstAliases we need to support "(rs1)" without an immediate. Doing this requires making CompressInstEmitter aware of sub-operands. I've chosen to use a flat list of operands in the CompressPats so each sub-operand is represented individually.
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-tablegen Author: Craig Topper (topperc) ChangesI'm looking into using sub-operands for memory operands. This would use MIOperandInfo to create a single operand that contains a register and immediate as sub-operands. We can treat this as a single operand for parsing and matching in the assembler. I believe this will provide some simplifications like removing the InstAliases we need to support "(rs1)" without an immediate. Doing this requires making CompressInstEmitter aware of sub-operands. I've chosen to use a flat list of operands in the CompressPats so each sub-operand is represented individually. Patch is 23.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133039.diff 1 Files Affected:
diff --git a/llvm/utils/TableGen/CompressInstEmitter.cpp b/llvm/utils/TableGen/CompressInstEmitter.cpp
index fe6db222f6b36..61b48f708626c 100644
--- a/llvm/utils/TableGen/CompressInstEmitter.cpp
+++ b/llvm/utils/TableGen/CompressInstEmitter.cpp
@@ -207,90 +207,109 @@ void CompressInstEmitter::addDagOperandMapping(const Record *Rec,
const CodeGenInstruction &Inst,
IndexedMap<OpData> &OperandMap,
bool IsSourceInst) {
+ unsigned NumMIOperands = 0;
+ for (const auto &Op : Inst.Operands)
+ NumMIOperands += Op.MINumOperands;
+ OperandMap.grow(NumMIOperands);
+
// TiedCount keeps track of the number of operands skipped in Inst
// operands list to get to the corresponding Dag operand. This is
// necessary because the number of operands in Inst might be greater
// than number of operands in the Dag due to how tied operands
// are represented.
unsigned TiedCount = 0;
- for (unsigned I = 0, E = Inst.Operands.size(); I != E; ++I) {
- int TiedOpIdx = Inst.Operands[I].getTiedRegister();
+ unsigned OpNo = 0;
+ for (const auto &Opnd : Inst.Operands) {
+ int TiedOpIdx = Opnd.getTiedRegister();
if (-1 != TiedOpIdx) {
// Set the entry in OperandMap for the tied operand we're skipping.
- OperandMap[I].Kind = OperandMap[TiedOpIdx].Kind;
- OperandMap[I].Data = OperandMap[TiedOpIdx].Data;
- TiedCount++;
+ OperandMap[OpNo].Kind = OperandMap[TiedOpIdx].Kind;
+ OperandMap[OpNo].Data = OperandMap[TiedOpIdx].Data;
+ ++OpNo;
+ ++TiedCount;
continue;
}
- if (const DefInit *DI = dyn_cast<DefInit>(Dag->getArg(I - TiedCount))) {
- if (DI->getDef()->isSubClassOf("Register")) {
- // Check if the fixed register belongs to the Register class.
- if (!validateRegister(DI->getDef(), Inst.Operands[I].Rec))
+ for (unsigned SubOp = 0; SubOp != Opnd.MINumOperands; ++SubOp, ++OpNo) {
+ unsigned DAGOpNo = OpNo - TiedCount;
+ const Record *OpndRec = Opnd.Rec;
+ if (Opnd.MINumOperands > 1)
+ OpndRec = cast<DefInit>(Opnd.MIOperandInfo->getArg(SubOp))->getDef();
+
+ if (const DefInit *DI = dyn_cast<DefInit>(Dag->getArg(DAGOpNo))) {
+ if (DI->getDef()->isSubClassOf("Register")) {
+ // Check if the fixed register belongs to the Register class.
+ if (!validateRegister(DI->getDef(), OpndRec))
+ PrintFatalError(Rec->getLoc(),
+ "Error in Dag '" + Dag->getAsString() +
+ "'Register: '" + DI->getDef()->getName() +
+ "' is not in register class '" +
+ OpndRec->getName() + "'");
+ OperandMap[OpNo].Kind = OpData::Reg;
+ OperandMap[OpNo].Data.Reg = DI->getDef();
+ continue;
+ }
+ // Validate that Dag operand type matches the type defined in the
+ // corresponding instruction. Operands in the input Dag pattern are
+ // allowed to be a subclass of the type specified in corresponding
+ // instruction operand instead of being an exact match.
+ if (!validateTypes(DI->getDef(), OpndRec, IsSourceInst))
PrintFatalError(Rec->getLoc(),
"Error in Dag '" + Dag->getAsString() +
- "'Register: '" + DI->getDef()->getName() +
- "' is not in register class '" +
- Inst.Operands[I].Rec->getName() + "'");
- OperandMap[I].Kind = OpData::Reg;
- OperandMap[I].Data.Reg = DI->getDef();
- continue;
- }
- // Validate that Dag operand type matches the type defined in the
- // corresponding instruction. Operands in the input Dag pattern are
- // allowed to be a subclass of the type specified in corresponding
- // instruction operand instead of being an exact match.
- if (!validateTypes(DI->getDef(), Inst.Operands[I].Rec, IsSourceInst))
- PrintFatalError(Rec->getLoc(),
- "Error in Dag '" + Dag->getAsString() + "'. Operand '" +
- Dag->getArgNameStr(I - TiedCount) + "' has type '" +
- DI->getDef()->getName() +
- "' which does not match the type '" +
- Inst.Operands[I].Rec->getName() +
- "' in the corresponding instruction operand!");
-
- OperandMap[I].Kind = OpData::Operand;
- } else if (const IntInit *II =
- dyn_cast<IntInit>(Dag->getArg(I - TiedCount))) {
- // Validate that corresponding instruction operand expects an immediate.
- if (Inst.Operands[I].Rec->isSubClassOf("RegisterClass"))
- PrintFatalError(
- Rec->getLoc(),
- "Error in Dag '" + Dag->getAsString() + "' Found immediate: '" +
- II->getAsString() +
- "' but corresponding instruction operand expected a register!");
- // No pattern validation check possible for values of fixed immediate.
- OperandMap[I].Kind = OpData::Imm;
- OperandMap[I].Data.Imm = II->getValue();
- LLVM_DEBUG(
- dbgs() << " Found immediate '" << II->getValue() << "' at "
- << (IsSourceInst ? "input " : "output ")
- << "Dag. No validation time check possible for values of "
- "fixed immediate.\n");
- } else
- llvm_unreachable("Unhandled CompressPat argument type!");
+ "'. Operand '" + Dag->getArgNameStr(DAGOpNo) +
+ "' has type '" + DI->getDef()->getName() +
+ "' which does not match the type '" +
+ OpndRec->getName() +
+ "' in the corresponding instruction operand!");
+
+ OperandMap[OpNo].Kind = OpData::Operand;
+ } else if (const IntInit *II = dyn_cast<IntInit>(Dag->getArg(DAGOpNo))) {
+ // Validate that corresponding instruction operand expects an immediate.
+ if (OpndRec->isSubClassOf("RegisterClass"))
+ PrintFatalError(Rec->getLoc(), "Error in Dag '" + Dag->getAsString() +
+ "' Found immediate: '" +
+ II->getAsString() +
+ "' but corresponding instruction "
+ "operand expected a register!");
+ // No pattern validation check possible for values of fixed immediate.
+ OperandMap[OpNo].Kind = OpData::Imm;
+ OperandMap[OpNo].Data.Imm = II->getValue();
+ LLVM_DEBUG(
+ dbgs() << " Found immediate '" << II->getValue() << "' at "
+ << (IsSourceInst ? "input " : "output ")
+ << "Dag. No validation time check possible for values of "
+ "fixed immediate.\n");
+ } else
+ llvm_unreachable("Unhandled CompressPat argument type!");
+ }
}
}
// Verify the Dag operand count is enough to build an instruction.
static bool verifyDagOpCount(const CodeGenInstruction &Inst, const DagInit *Dag,
bool IsSource) {
- if (Dag->getNumArgs() == Inst.Operands.size())
+ unsigned NumMIOperands = 0;
+ for (const auto &Op : Inst.Operands)
+ NumMIOperands += Op.MINumOperands;
+
+ if (Dag->getNumArgs() == NumMIOperands)
return true;
+
// Source instructions are non compressed instructions and don't have tied
// operands.
if (IsSource)
PrintFatalError(Inst.TheDef->getLoc(),
"Input operands for Inst '" + Inst.TheDef->getName() +
"' and input Dag operand count mismatch");
+
// The Dag can't have more arguments than the Instruction.
- if (Dag->getNumArgs() > Inst.Operands.size())
+ if (Dag->getNumArgs() > NumMIOperands)
PrintFatalError(Inst.TheDef->getLoc(),
"Inst '" + Inst.TheDef->getName() +
"' and Dag operand count mismatch");
// The Instruction might have tied operands so the Dag might have
// a fewer operand count.
- unsigned RealCount = Inst.Operands.size();
+ unsigned RealCount = NumMIOperands;
for (const auto &Operand : Inst.Operands)
if (Operand.getTiedRegister() != -1)
--RealCount;
@@ -366,42 +385,47 @@ void CompressInstEmitter::createInstOperandMapping(
// operands list to get to the corresponding Dag operand.
unsigned TiedCount = 0;
LLVM_DEBUG(dbgs() << " Operand mapping:\n Source Dest\n");
- for (unsigned I = 0, E = DestInst.Operands.size(); I != E; ++I) {
- int TiedInstOpIdx = DestInst.Operands[I].getTiedRegister();
+ unsigned OpNo = 0;
+ for (const auto &Operand : DestInst.Operands) {
+ int TiedInstOpIdx = Operand.getTiedRegister();
if (TiedInstOpIdx != -1) {
++TiedCount;
- DestOperandMap[I].Data = DestOperandMap[TiedInstOpIdx].Data;
- DestOperandMap[I].Kind = DestOperandMap[TiedInstOpIdx].Kind;
- if (DestOperandMap[I].Kind == OpData::Operand)
+ DestOperandMap[OpNo].Data = DestOperandMap[TiedInstOpIdx].Data;
+ DestOperandMap[OpNo].Kind = DestOperandMap[TiedInstOpIdx].Kind;
+ if (DestOperandMap[OpNo].Kind == OpData::Operand)
// No need to fill the SourceOperandMap here since it was mapped to
// destination operand 'TiedInstOpIdx' in a previous iteration.
- LLVM_DEBUG(dbgs() << " " << DestOperandMap[I].Data.Operand
- << " ====> " << I
+ LLVM_DEBUG(dbgs() << " " << DestOperandMap[OpNo].Data.Operand
+ << " ====> " << OpNo
<< " Dest operand tied with operand '"
<< TiedInstOpIdx << "'\n");
+ ++OpNo;
continue;
}
- // Skip fixed immediates and registers, they were handled in
- // addDagOperandMapping.
- if (DestOperandMap[I].Kind != OpData::Operand)
- continue;
- unsigned DagArgIdx = I - TiedCount;
- StringMap<unsigned>::iterator SourceOp =
- SourceOperands.find(DestDag->getArgNameStr(DagArgIdx));
- if (SourceOp == SourceOperands.end())
- PrintFatalError(Rec->getLoc(),
- "Output Dag operand '" +
- DestDag->getArgNameStr(DagArgIdx) +
- "' has no matching input Dag operand.");
-
- assert(DestDag->getArgNameStr(DagArgIdx) ==
- SourceDag->getArgNameStr(SourceOp->getValue()) &&
- "Incorrect operand mapping detected!\n");
- DestOperandMap[I].Data.Operand = SourceOp->getValue();
- SourceOperandMap[SourceOp->getValue()].Data.Operand = I;
- LLVM_DEBUG(dbgs() << " " << SourceOp->getValue() << " ====> " << I
- << "\n");
+ for (unsigned SubOp = 0; SubOp != Operand.MINumOperands; ++SubOp, ++OpNo) {
+ // Skip fixed immediates and registers, they were handled in
+ // addDagOperandMapping.
+ if (DestOperandMap[OpNo].Kind != OpData::Operand)
+ continue;
+
+ unsigned DagArgIdx = OpNo - TiedCount;
+ StringMap<unsigned>::iterator SourceOp =
+ SourceOperands.find(DestDag->getArgNameStr(DagArgIdx));
+ if (SourceOp == SourceOperands.end())
+ PrintFatalError(Rec->getLoc(),
+ "Output Dag operand '" +
+ DestDag->getArgNameStr(DagArgIdx) +
+ "' has no matching input Dag operand.");
+
+ assert(DestDag->getArgNameStr(DagArgIdx) ==
+ SourceDag->getArgNameStr(SourceOp->getValue()) &&
+ "Incorrect operand mapping detected!\n");
+ DestOperandMap[OpNo].Data.Operand = SourceOp->getValue();
+ SourceOperandMap[SourceOp->getValue()].Data.Operand = OpNo;
+ LLVM_DEBUG(dbgs() << " " << SourceOp->getValue() << " ====> " << OpNo
+ << "\n");
+ }
}
}
@@ -459,13 +483,11 @@ void CompressInstEmitter::evaluateCompressPat(const Record *Rec) {
// Fill the mapping from the source to destination instructions.
IndexedMap<OpData> SourceOperandMap;
- SourceOperandMap.grow(SourceInst.Operands.size());
// Create a mapping between source Dag operands and source Inst operands.
addDagOperandMapping(Rec, SourceDag, SourceInst, SourceOperandMap,
/*IsSourceInst*/ true);
IndexedMap<OpData> DestOperandMap;
- DestOperandMap.grow(DestInst.Operands.size());
// Create a mapping between destination Dag operands and destination Inst
// operands.
addDagOperandMapping(Rec, DestDag, DestInst, DestOperandMap,
@@ -720,7 +742,7 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
// Start Source Inst operands validation.
unsigned OpNo = 0;
- for (OpNo = 0; OpNo < Source.Operands.size(); ++OpNo) {
+ for (const auto &SourceOperand : Source.Operands) {
if (SourceOperandMap[OpNo].TiedOpIdx != -1) {
if (Source.Operands[OpNo].Rec->isSubClassOf("RegisterClass"))
CondStream.indent(6)
@@ -732,25 +754,29 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
else
PrintFatalError("Unexpected tied operand types!\n");
}
- // Check for fixed immediates\registers in the source instruction.
- switch (SourceOperandMap[OpNo].Kind) {
- case OpData::Operand:
- // We don't need to do anything for source instruction operand checks.
- break;
- case OpData::Imm:
- CondStream.indent(6)
- << "(MI.getOperand(" << OpNo << ").isImm()) &&\n"
- << " (MI.getOperand(" << OpNo
- << ").getImm() == " << SourceOperandMap[OpNo].Data.Imm << ") &&\n";
- break;
- case OpData::Reg: {
- const Record *Reg = SourceOperandMap[OpNo].Data.Reg;
- CondStream.indent(6)
- << "(MI.getOperand(" << OpNo << ").isReg()) &&\n"
- << " (MI.getOperand(" << OpNo << ").getReg() == " << TargetName
- << "::" << Reg->getName() << ") &&\n";
- break;
- }
+ for (unsigned SubOp = 0; SubOp != SourceOperand.MINumOperands; ++SubOp) {
+ // Check for fixed immediates\registers in the source instruction.
+ switch (SourceOperandMap[OpNo].Kind) {
+ case OpData::Operand:
+ // We don't need to do anything for source instruction operand checks.
+ break;
+ case OpData::Imm:
+ CondStream.indent(6)
+ << "(MI.getOperand(" << OpNo << ").isImm()) &&\n"
+ << " (MI.getOperand(" << OpNo
+ << ").getImm() == " << SourceOperandMap[OpNo].Data.Imm
+ << ") &&\n";
+ break;
+ case OpData::Reg: {
+ const Record *Reg = SourceOperandMap[OpNo].Data.Reg;
+ CondStream.indent(6) << "(MI.getOperand(" << OpNo << ").isReg()) &&\n"
+ << " (MI.getOperand(" << OpNo
+ << ").getReg() == " << TargetName
+ << "::" << Reg->getName() << ") &&\n";
+ break;
+ }
+ }
+ ++OpNo;
}
}
CodeStream.indent(6) << "// " << Dest.AsmString << "\n";
@@ -760,91 +786,100 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
OpNo = 0;
for (const auto &DestOperand : Dest.Operands) {
CodeStream.indent(6) << "// Operand: " << DestOperand.Name << "\n";
- switch (DestOperandMap[OpNo].Kind) {
- case OpData::Operand: {
- unsigned OpIdx = DestOperandMap[OpNo].Data.Operand;
- // Check that the operand in the Source instruction fits
- // the type for the Dest instruction.
- if (DestOperand.Rec->isSubClassOf("RegisterClass") ||
- DestOperand.Rec->isSubClassOf("RegisterOperand")) {
- auto *ClassRec = DestOperand.Rec->isSubClassOf("RegisterClass")
- ? DestOperand.Rec
- : DestOperand.Rec->getValueAsDef("RegClass");
- // This is a register operand. Check the register class.
- // Don't check register class if this is a tied operand, it was done
- // for the operand its tied to.
- if (DestOperand.getTiedRegister() == -1) {
- CondStream.indent(6) << "MI.getOperand(" << OpIdx << ").isReg()";
- if (EType == EmitterType::CheckCompress)
- CondStream << " && MI.getOperand(" << OpIdx
- << ").getReg().isPhysical()";
- CondStream << " &&\n"
- << indent(6) << TargetName << "MCRegisterClasses["
- << TargetName << "::" << ClassRec->getName()
- << "RegClassID].contains(MI.getOperand(" << OpIdx
- << ").getReg()) &&\n";
- }
- if (CompressOrUncompress)
- CodeStream.indent(6)
- << "OutInst.addOperand(MI.getOperand(" << OpIdx << "));\n";
- } else {
- // Handling immediate operands.
+ for (unsigned SubOp = 0; SubOp != DestOperand.MINumOperands; ++SubOp) {
+ const Record *DestRec = DestOperand.Rec;
+
+ if (DestOperand.MINumOperands > 1)
+ DestRec =
+ cast<DefInit>(DestOperand.MIOperandInfo->getArg(SubOp))->getDef();
+
+ switch (DestOperandMap[OpNo].Kind) {
+ case OpData::Operand: {
+ unsigned OpIdx = DestOperandMap[OpNo].Data.Operand;
+ // Check that the operand in the Source instruction fits
+ // the type for the Dest instruction.
+ if (DestRec->isSubClassOf("RegisterClass") ||
+ DestRec->isSubClassOf("RegisterOperand")) {
+ auto *ClassRec = DestRec->isSubClassOf("RegisterClass")
+ ? DestRec
+ : DestRec->getValueAsDef("RegClass");
+ // This is a register operand. Check the register class.
+ // Don't check register class if this is a tied operand, it was done
+ // for the operand its tied to.
+ if (DestOperand.getTiedRegister() == -1) {
+ CondStream.indent(6) << "MI.getOperand(" << OpIdx << ").isReg()";
+ if (EType == EmitterType::CheckCompress)
+ CondStream << " && MI.getOperand(" << OpIdx
+ << ").getReg().isPhysical()";
+ CondStream << " &&\n"
+ << indent(6) << TargetName << "MCRegisterClasses["
+ << TargetName << "::" << ClassRec->getName()
+ << "RegClassID].contains(MI.getOperand(" << OpIdx
+ << ").getReg()) &&\n";
+ }
+
+ if (CompressOrUncompress)
+ CodeStream.indent(6)
+ << "OutInst.addOperand(MI.getOperand(" << OpIdx << "));\n";
+ } else {
+ // Handling immediate operands.
+ if (CompressOrUncompress) {
+ unsigned Entry = getPredicates(MCOpPredicateMap, MCOpPredicates,
+ DestRec, "MCOperandPredicate");
+ CondStream.indent(6) << ValidatorName << "("
+ << "MI.getOperand(" << OpIdx << "), STI, "
+ << Entry << ") &&\n";
+ } else {
+ unsigned Entry =
+ getPredicates(ImmLeafPredicateMap, ImmLeafPredicates, DestRec,
+ "ImmediateCode");
+ CondStream.indent(6)
+ << "MI.getOperand(" << OpIdx << ").isImm() &&\n";
+ CondStream.indent(6) << TargetName << "ValidateMachineOperand("
+ << "MI.getOperand(" << OpIdx << "), &STI, "
+ << Entry << ") &&\n";
+ }
+ if (CompressOrUncompress)
+ CodeStream.indent(6)
+ << "OutInst.addOperand(MI.getOperand(" << OpIdx << "));\n";
+ }
+ break;
+ }
+ case OpData::Imm: {
if (CompressOrUncompress) {
- unsigned Entry =
- getPredicates(MCOpPredicateMap, MCOpPredicates, DestOperand.Rec,
- "MCOperandPredicate");
+ ...
[truncated]
|
wangpc-pp
reviewed
Mar 26, 2025
topperc
commented
Mar 26, 2025
| // CHECK-NEXT: // Operand: dst | ||
| // CHECK-NEXT: OutInst.addOperand(MI.getOperand(0)); | ||
| // CHECK-NEXT: // Operand: addr | ||
| // CHECK-NEXT: OutInst.addOperand(MI.getOperand(1)); |
There was a problem hiding this comment.
Here you can see that one named operand from the instruction has multiple addOperand calls for the sub-operands
wangpc-pp
approved these changes
Mar 27, 2025
mshockwave
reviewed
Mar 27, 2025
lenary
approved these changes
Mar 28, 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.
I'm looking into using sub-operands for memory operands. This would use MIOperandInfo to create a single operand that contains a register and immediate as sub-operands. We can treat this as a single operand for parsing and matching in the assembler. I believe this will provide some simplifications like removing the InstAliases we need to support "(rs1)" without an immediate.
Doing this requires making CompressInstEmitter aware of sub-operands.
I've chosen to use a flat list of operands in the CompressPats so each sub-operand is represented individually.