Skip to content

[TableGen] Use llvm::enumerate in one place (NFC) #136399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 19, 2025

Conversation

s-barannikov
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Apr 19, 2025

@llvm/pr-subscribers-tablegen

Author: Sergei Barannikov (s-barannikov)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/136399.diff

1 Files Affected:

  • (modified) llvm/utils/TableGen/Common/CodeGenInstAlias.cpp (+11-13)
diff --git a/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp b/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
index 5537a2fa8b980..94c0d51faf2e9 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
@@ -200,17 +200,15 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
 
   // Decode and validate the arguments of the result.
   unsigned AliasOpNo = 0;
-  for (unsigned i = 0, e = ResultInst->Operands.size(); i != e; ++i) {
-
+  for (auto [OpIdx, OpInfo] : enumerate(ResultInst->Operands)) {
     // Tied registers don't have an entry in the result dag unless they're part
     // of a complex operand, in which case we include them anyways, as we
     // don't have any other way to specify the whole operand.
-    if (ResultInst->Operands[i].MINumOperands == 1 &&
-        ResultInst->Operands[i].getTiedRegister() != -1) {
+    if (OpInfo.MINumOperands == 1 && OpInfo.getTiedRegister() != -1) {
       // Tied operands of different RegisterClass should be explicit within an
       // instruction's syntax and so cannot be skipped.
-      int TiedOpNum = ResultInst->Operands[i].getTiedRegister();
-      if (ResultInst->Operands[i].Rec->getName() ==
+      int TiedOpNum = OpInfo.getTiedRegister();
+      if (OpInfo.Rec->getName() ==
           ResultInst->Operands[TiedOpNum].Rec->getName())
         continue;
     }
@@ -218,8 +216,8 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
     if (AliasOpNo >= Result->getNumArgs())
       PrintFatalError(R->getLoc(), "not enough arguments for instruction!");
 
-    const Record *InstOpRec = ResultInst->Operands[i].Rec;
-    unsigned NumSubOps = ResultInst->Operands[i].MINumOperands;
+    const Record *InstOpRec = OpInfo.Rec;
+    unsigned NumSubOps = OpInfo.MINumOperands;
     ResultOperand ResOp(static_cast<int64_t>(0));
     if (tryAliasOpMatch(Result, AliasOpNo, InstOpRec, (NumSubOps > 1),
                         R->getLoc(), T, ResOp)) {
@@ -229,12 +227,12 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
                              InstOpRec->getValueAsDef("ParserMatchClass")
                                      ->getValueAsString("Name") != "Imm")) {
         ResultOperands.push_back(std::move(ResOp));
-        ResultInstOperandIndex.emplace_back(i, -1);
+        ResultInstOperandIndex.emplace_back(OpIdx, -1);
         ++AliasOpNo;
 
         // Otherwise, we need to match each of the suboperands individually.
       } else {
-        const DagInit *MIOI = ResultInst->Operands[i].MIOperandInfo;
+        const DagInit *MIOI = OpInfo.MIOperandInfo;
         for (unsigned SubOp = 0; SubOp != NumSubOps; ++SubOp) {
           const Record *SubRec = cast<DefInit>(MIOI->getArg(SubOp))->getDef();
 
@@ -244,7 +242,7 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
               Result->getArgName(AliasOpNo)->getAsUnquotedString() + "." +
                   MIOI->getArgName(SubOp)->getAsUnquotedString(),
               SubRec);
-          ResultInstOperandIndex.emplace_back(i, SubOp);
+          ResultInstOperandIndex.emplace_back(OpIdx, SubOp);
         }
         ++AliasOpNo;
       }
@@ -254,7 +252,7 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
     // If the argument did not match the instruction operand, and the operand
     // is composed of multiple suboperands, try matching the suboperands.
     if (NumSubOps > 1) {
-      const DagInit *MIOI = ResultInst->Operands[i].MIOperandInfo;
+      const DagInit *MIOI = OpInfo.MIOperandInfo;
       for (unsigned SubOp = 0; SubOp != NumSubOps; ++SubOp) {
         if (AliasOpNo >= Result->getNumArgs())
           PrintFatalError(R->getLoc(), "not enough arguments for instruction!");
@@ -262,7 +260,7 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)
         if (tryAliasOpMatch(Result, AliasOpNo, SubRec, false, R->getLoc(), T,
                             ResOp)) {
           ResultOperands.push_back(ResOp);
-          ResultInstOperandIndex.emplace_back(i, SubOp);
+          ResultInstOperandIndex.emplace_back(OpIdx, SubOp);
           ++AliasOpNo;
         } else {
           PrintFatalError(

@@ -200,26 +200,24 @@ CodeGenInstAlias::CodeGenInstAlias(const Record *R, const CodeGenTarget &T)

// Decode and validate the arguments of the result.
unsigned AliasOpNo = 0;
for (unsigned i = 0, e = ResultInst->Operands.size(); i != e; ++i) {

for (auto [OpIdx, OpInfo] : enumerate(ResultInst->Operands)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use const auto &[OpIdx, OpInfo] here to avoid making a copy?

Copy link
Contributor Author

@s-barannikov s-barannikov Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're concerned about copying OpInfo, then there is no copy, AFAICT.
static_assert(std::is_reference_v<decltype(OpInfo)>); doesn't trigger.
I never learned how structured bindings exactly work, but it must be something with the fact that & applies to the structure itself and not to its elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the size of the structure appears to be just 16 bytes.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no copying here. This style is used all over mlir with enumerate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kazutakahirata kazutakahirata requested a review from kuhar April 19, 2025 04:29
@s-barannikov s-barannikov merged commit ee4c8b5 into llvm:main Apr 19, 2025
13 checks passed
@s-barannikov s-barannikov deleted the tablegen/inst-alias-enumerate branch April 26, 2025 19:10
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants