[NFC][TableGen] Code cleanup in Wasm disassember emitter #135992

jurahul · 2025-04-16T17:26:27Z

Use range for loop to iterate over instructions.
Emit generated code in anonymous namespace instead of llvm and reduce the scope of this to just the type declarations.
Emit generated tables as static constexpr
Replace code to search in operand table with std::search.
Skip the last "null" entry in PrefixTable and use range for loop to search PrefixTable in the .cpp code.
Do not generate WebAssemblyInstructionTableSize definition as its already defined in the .cpp file.
Remove {} for single statement loop/if/else bodies.

llvmbot · 2025-04-16T20:04:53Z

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-backend-webassembly

Author: Rahul Joshi (jurahul)

Changes

Use range for loop to iterate over instructions.
Emit generated code in anonymous namespace instead of llvm, and reduce the scope of this to just the type declarations.
Emit generated tables as static constexpr
Replace code to search in operand table with std::search.
Skip the last "null" entry in PrefixTable and use range for loop to search PrefixTable in the .cpp code.
Do not generate WebAssemblyInstructionTableSize definition as its already defined in the .cpp file.
Remove {} for single statement loop/if/else bodies.

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

2 Files Affected:

(modified) llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp (+5-5)
(modified) llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp (+41-46)

diff --git a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
index 7bee4af86b52a..0399f9d38e4eb 100644
--- a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
+++ b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
@@ -38,9 +38,9 @@ using DecodeStatus = MCDisassembler::DecodeStatus;
 
 #include "WebAssemblyGenDisassemblerTables.inc"
 
-namespace {
 static constexpr int WebAssemblyInstructionTableSize = 256;
 
+namespace {
 class WebAssemblyDisassembler final : public MCDisassembler {
   std::unique_ptr<const MCInstrInfo> MCII;
 
@@ -171,10 +171,10 @@ MCDisassembler::DecodeStatus WebAssemblyDisassembler::getInstruction(
   // If this is a prefix byte, indirect to another table.
   if (WasmInst->ET == ET_Prefix) {
     WasmInst = nullptr;
-    // Linear search, so far only 2 entries.
-    for (auto PT = PrefixTable; PT->Table; PT++) {
-      if (PT->Prefix == Opc) {
-        WasmInst = PT->Table;
+    // Linear search, so far only 4 entries.
+    for (const auto &[Prefix, Table] : PrefixTable) {
+      if (Prefix == Opc) {
+        WasmInst = Table;
         break;
       }
     }
diff --git a/llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp b/llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp
index 5aa573ac857dc..cf82176bfdb99 100644
--- a/llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp
+++ b/llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp
@@ -1,4 +1,4 @@
-//===- WebAssemblyDisassemblerEmitter.cpp - Disassembler tables -*- C++ -*-===//
+//===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,6 +16,7 @@
 #include "WebAssemblyDisassemblerEmitter.h"
 #include "Common/CodeGenInstruction.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/iterator.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TableGen/Record.h"
 
@@ -29,8 +30,8 @@ void llvm::emitWebAssemblyDisassemblerTables(
   std::map<unsigned,
            std::map<unsigned, std::pair<unsigned, const CodeGenInstruction *>>>
       OpcodeTable;
-  for (unsigned I = 0; I != NumberedInstructions.size(); ++I) {
-    const CodeGenInstruction &CGI = *NumberedInstructions[I];
+  for (const auto &[Idx, CGI] :
+       enumerate(make_pointee_range(NumberedInstructions))) {
     const Record &Def = *CGI.TheDef;
     if (!Def.getValue("Inst"))
       continue;
@@ -75,27 +76,31 @@ void llvm::emitWebAssemblyDisassemblerTables(
       }
     }
     // Set this instruction as the one to use.
-    CGIP = {I, &CGI};
+    CGIP = {Idx, &CGI};
   }
-  OS << "#include \"MCTargetDesc/WebAssemblyMCTargetDesc.h\"\n";
-  OS << "\n";
-  OS << "namespace llvm {\n\n";
-  OS << "static constexpr int WebAssemblyInstructionTableSize = ";
-  OS << WebAssemblyInstructionTableSize << ";\n\n";
-  OS << "enum EntryType : uint8_t { ";
-  OS << "ET_Unused, ET_Prefix, ET_Instruction };\n\n";
-  OS << "struct WebAssemblyInstruction {\n";
-  OS << "  uint16_t Opcode;\n";
-  OS << "  EntryType ET;\n";
-  OS << "  uint8_t NumOperands;\n";
-  OS << "  uint16_t OperandStart;\n";
-  OS << "};\n\n";
+
+  OS << R"(
+#include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
+
+namespace {
+enum EntryType : uint8_t { ET_Unused, ET_Prefix, ET_Instruction };
+
+struct WebAssemblyInstruction {
+  uint16_t Opcode;
+  EntryType ET;
+  uint8_t NumOperands;
+  uint16_t OperandStart;
+};
+} // end anonymous namespace
+
+)";
+
   std::vector<std::string> OperandTable, CurOperandList;
   // Output one table per prefix.
   for (const auto &[Prefix, Table] : OpcodeTable) {
     if (Table.empty())
       continue;
-    OS << "WebAssemblyInstruction InstructionTable" << Prefix;
+    OS << "static constexpr WebAssemblyInstruction InstructionTable" << Prefix;
     OS << "[] = {\n";
     for (unsigned I = 0; I < WebAssemblyInstructionTableSize; I++) {
       auto InstIt = Table.find(I);
@@ -116,53 +121,43 @@ void llvm::emitWebAssemblyDisassemblerTables(
         }
         // See if we already have stored this sequence before. This is not
         // strictly necessary but makes the table really small.
-        size_t OperandStart = OperandTable.size();
-        if (CurOperandList.size() <= OperandTable.size()) {
-          for (size_t J = 0; J <= OperandTable.size() - CurOperandList.size();
-               ++J) {
-            size_t K = 0;
-            for (; K < CurOperandList.size(); ++K) {
-              if (OperandTable[J + K] != CurOperandList[K])
-                break;
-            }
-            if (K == CurOperandList.size()) {
-              OperandStart = J;
-              break;
-            }
-          }
-        }
+        auto SearchI =
+            std::search(OperandTable.begin(), OperandTable.end(),
+                        CurOperandList.begin(), CurOperandList.end());
+        OS << std::distance(OperandTable.begin(), SearchI);
         // Store operands if no prior occurrence.
-        if (OperandStart == OperandTable.size()) {
+        if (SearchI == OperandTable.end())
           llvm::append_range(OperandTable, CurOperandList);
-        }
-        OS << OperandStart;
       } else {
         auto PrefixIt = OpcodeTable.find(I);
         // If we have a non-empty table for it that's not 0, this is a prefix.
-        if (PrefixIt != OpcodeTable.end() && I && !Prefix) {
+        if (PrefixIt != OpcodeTable.end() && I && !Prefix)
           OS << "  { 0, ET_Prefix, 0, 0";
-        } else {
+        else
           OS << "  { 0, ET_Unused, 0, 0";
-        }
       }
       OS << "  },\n";
     }
     OS << "};\n\n";
   }
   // Create a table of all operands:
-  OS << "const uint8_t OperandTable[] = {\n";
-  for (auto &Op : OperandTable) {
+  OS << "static constexpr uint8_t OperandTable[] = {\n";
+  for (const auto &Op : OperandTable)
     OS << "  " << Op << ",\n";
-  }
   OS << "};\n\n";
+
   // Create a table of all extension tables:
-  OS << "struct { uint8_t Prefix; const WebAssemblyInstruction *Table; }\n";
-  OS << "PrefixTable[] = {\n";
+  OS << R"(
+static constexpr struct {
+  uint8_t Prefix;
+  const WebAssemblyInstruction *Table;
+} PrefixTable[] = {
+)";
+
   for (const auto &[Prefix, Table] : OpcodeTable) {
     if (Table.empty() || !Prefix)
       continue;
     OS << "  { " << Prefix << ", InstructionTable" << Prefix << " },\n";
   }
-  OS << "  { 0, nullptr }\n};\n\n";
-  OS << "} // end namespace llvm\n";
+  OS << "};\n";
 }

llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp

jurahul · 2025-04-16T21:02:19Z

Per the latest coding standard, we don’t need it anymore so I took the opportunity to remove it.

…

On Wed, Apr 16, 2025 at 2:01 PM Craig Topper ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp <#135992 (comment)>: > @@ -1,4 +1,4 @@ -//===- WebAssemblyDisassemblerEmitter.cpp - Disassembler tables -*- C++ -*-===// +//===----------------------------------------------------------------------===// What happened to the file name? — Reply to this email directly, view it on GitHub <#135992 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRMUB44CNQVSTMEZYRVBBD2Z3AIZAVCNFSM6AAAAAB3I3SSQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONZTHA2DSMRQG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

jurahul · 2025-04-16T21:09:14Z

Yeah it was duplicated and I removed it.

…

On Wed, Apr 16, 2025 at 2:08 PM Craig Topper ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp <#135992 (comment)>: > @@ -38,9 +38,9 @@ using DecodeStatus = MCDisassembler::DecodeStatus; #include "WebAssemblyGenDisassemblerTables.inc" -namespace { static constexpr int WebAssemblyInstructionTableSize = 256; Oh nevermind you removed it from the generated file — Reply to this email directly, view it on GitHub <#135992 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRMUB4AAXTFIU6TJNZTZC32Z3BEJAVCNFSM6AAAAAB3I3SSQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONZTHA3DEMRQGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp

- Use range for loop to iterate over instructions. - Emit generated code in anonymous namespace instead of `llvm`, and reduce the scope of this to just the type declarations. - Emit generated tables as static constexpr - Replace code to search in operand table with `std::search`. - Skip the last "null" entry in PrefixTable and use range for loop to search PrefixTable in the .cpp code. - Do not generate `WebAssemblyInstructionTableSize` definition as its already defined in the .cpp file. - Remove {} for single statement loop/if/else bodies.

- Use range for loop to iterate over instructions. - Emit generated code in anonymous namespace instead of `llvm` and reduce the scope of this to just the type declarations. - Emit generated tables as static constexpr - Replace code to search in operand table with `std::search`. - Skip the last "null" entry in PrefixTable and use range for loop to search PrefixTable in the .cpp code. - Do not generate `WebAssemblyInstructionTableSize` definition as its already defined in the .cpp file. - Remove {} for single statement loop/if/else bodies.

jurahul force-pushed the nfc_webasm_dis_emitter_cleanup branch from 34ffa4a to 6ab98de Compare April 16, 2025 17:29

jurahul marked this pull request as ready for review April 16, 2025 20:04

jurahul requested a review from jayfoad April 16, 2025 20:04

llvmbot added backend:WebAssembly tablegen labels Apr 16, 2025

jurahul requested review from sunfishcode and MaskRay April 16, 2025 20:04

topperc reviewed Apr 16, 2025

View reviewed changes

llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp Show resolved Hide resolved

topperc reviewed Apr 16, 2025

View reviewed changes

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp Show resolved Hide resolved

MaskRay approved these changes Apr 17, 2025

View reviewed changes

llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp Outdated Show resolved Hide resolved

jurahul force-pushed the nfc_webasm_dis_emitter_cleanup branch from 6ab98de to 36103d5 Compare April 17, 2025 19:46

jurahul merged commit b3a53cc into llvm:main Apr 17, 2025
11 checks passed

jurahul deleted the nfc_webasm_dis_emitter_cleanup branch April 17, 2025 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][TableGen] Code cleanup in Wasm disassember emitter #135992

[NFC][TableGen] Code cleanup in Wasm disassember emitter #135992

Uh oh!

jurahul commented Apr 16, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

jurahul commented Apr 16, 2025 via email

Uh oh!

Uh oh!

jurahul commented Apr 16, 2025 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[NFC][TableGen] Code cleanup in Wasm disassember emitter #135992

[NFC][TableGen] Code cleanup in Wasm disassember emitter #135992

Uh oh!

Conversation

jurahul commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jurahul commented Apr 16, 2025 via email

Uh oh!

Uh oh!

jurahul commented Apr 16, 2025 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jurahul commented Apr 16, 2025 •

edited

Loading

llvmbot commented Apr 16, 2025 •

edited

Loading