[TableGen] Accurately calculate where the source variable ops start in PseudoLoweringEmitter::emitLoweringEmitter. #135465
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
…art in PseudoLoweringEmitter::emitLoweringEmitter. The code was using the number or source operands plus one. The plus one seems to be an ARM specific value accounting for one of the source operands having 2 sub operands. No other target in tree uses PseudLowering with variadic instructions so this worked. This patch replaces it with a proper count of the number of sub operands of all operands. While there I update the loop to use MIOperandNo so we don't need to count up the sub operands as we go.
|
@llvm/pr-subscribers-tablegen Author: Craig Topper (topperc) ChangesThe code was using the number or source operands plus one. The plus one seems to be an ARM specific value accounting for one of the source operands having 2 sub operands. No other target in tree uses PseudoLowering with variadic instructions so this worked. This patch replaces it with a proper count of the number of sub operands of all operands. While there I update the loop to use MIOperandNo so we don't need to count up the sub operands as we go. Full diff: https://github.com/llvm/llvm-project/pull/135465.diff 1 Files Affected:
diff --git a/llvm/utils/TableGen/PseudoLoweringEmitter.cpp b/llvm/utils/TableGen/PseudoLoweringEmitter.cpp
index 6d39d1496c09e..2c8f2f08f0536 100644
--- a/llvm/utils/TableGen/PseudoLoweringEmitter.cpp
+++ b/llvm/utils/TableGen/PseudoLoweringEmitter.cpp
@@ -247,9 +247,9 @@ void PseudoLoweringEmitter::emitLoweringEmitter(raw_ostream &o) {
// FIXME: Instruction operands with defaults values (predicates and cc_out
// in ARM, for example shouldn't need explicit values in the
// expansion DAG.
- unsigned MIOpNo = 0;
for (const auto &DestOperand : Dest.Operands) {
o << " // Operand: " << DestOperand.Name << "\n";
+ unsigned MIOpNo = DestOperand.MIOperandNo;
for (unsigned i = 0, e = DestOperand.MINumOperands; i != e; ++i) {
switch (Expansion.OperandMap[MIOpNo + i].Kind) {
case OpData::Operand:
@@ -277,12 +277,13 @@ void PseudoLoweringEmitter::emitLoweringEmitter(raw_ostream &o) {
}
}
}
- MIOpNo += DestOperand.MINumOperands;
}
if (Dest.Operands.isVariadic) {
- MIOpNo = Source.Operands.size() + 1;
+ unsigned LastOpNo = 0;
+ for (const auto &Op : Source.Operands)
+ LastOpNo += Op.MINumOperands;
o << " // variable_ops\n";
- o << " for (unsigned i = " << MIOpNo
+ o << " for (unsigned i = " << LastOpNo
<< ", e = MI->getNumOperands(); i != e; ++i)\n"
<< " if (lowerOperand(MI->getOperand(i), MCOp))\n"
<< " Inst.addOperand(MCOp);\n";
|
arsenm
approved these changes
Apr 12, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/15910 Here is the relevant piece of the build log for the reference |
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
…n PseudoLoweringEmitter::emitLoweringEmitter. (llvm#135465) The code was using the number of source operands plus one. The plus one seems to be an ARM specific value accounting for one of the source operands having 2 sub operands. No other target in tree uses PseudoLowering with variadic instructions so this worked. This patch replaces it with a proper count of the number of sub operands of all operands. While there I update the loop to use MIOperandNo so we don't need to count up the sub operands as we go.
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.
The code was using the number of source operands plus one. The plus one seems to be an ARM specific value accounting for one of the source operands having 2 sub operands. No other target in tree uses PseudoLowering with variadic instructions so this worked.
This patch replaces it with a proper count of the number of sub operands of all operands. While there I update the loop to use MIOperandNo so we don't need to count up the sub operands as we go.