Skip to content

[DecoderEmitter] Eliminate DecodeComplete parameter to decodeToMCInst #159130

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[DecoderEmitter] Eliminate DecodeComplete parameter to decodeToMCInst #159130

wants to merge 1 commit into from

Conversation

@jurahul
Copy link
Contributor

@jurahul jurahul commented Sep 16, 2025 •
edited
Loading

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst. As a result, we can just use the return DecodeStatus from decodeToMCInst to decide whether to terminate the decoding and return or continue trying.

Also update the DecodeStatus combining table documentation to use the term "SoftFail" instead of "Unpredictable".

@jurahul
[DecoderEmitter] Eliminate DecodeComplete parameter to `decodeToMCI…
88b5445 
…nst`

Currently, the code generated by decoder emitter always set
`DeocdeComplete` to false when returning Fail from `decodeToMCInst`. As
a result, we can just use the return `DecodeStatus` from
`decodeToMCInst` to decide whether to terminate the decoding and
return or continue trying.
@jurahul jurahul force-pushed the decoder_emitter_eliminate_decode_complete branch from c91b80d to 88b5445 Compare September 16, 2025 17:08
@jurahul jurahul mentioned this pull request Sep 16, 2025
Draft
@jurahul jurahul marked this pull request as ready for review September 16, 2025 17:55
@llvmbot llvmbot added tablegen llvm:mc Machine (object) code labels Sep 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2025

@llvm/pr-subscribers-llvm-mc

Author: Rahul Joshi (jurahul)

Changes

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst. As a result, we can just use the return DecodeStatus from decodeToMCInst to decide whether to terminate the decoding and return or continue trying.

Also update the DecodeStatus combining table documentation to use the term "SoftFail" instead of "Unpredictable".

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

7 Files Affected:

  • (modified) llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h (+5-5)
  • (modified) llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td (+6-6)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission.td (+2-2)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td (+4-4)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td (+2-2)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td (+2-2)
  • (modified) llvm/utils/TableGen/DecoderEmitter.cpp (+9-20)
diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index cae2fbcac1fef..3d10380fa1d46 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -98,11 +98,11 @@ class LLVM_ABI MCDisassembler {
   /// from Success->SoftFail ->Fail can be done with a simple
   /// bitwise-AND:
   ///
-  ///   LEFT & TOP =  | Success       Unpredictable   Fail
-  ///   --------------+-----------------------------------
-  ///   Success       | Success       Unpredictable   Fail
-  ///   Unpredictable | Unpredictable Unpredictable   Fail
-  ///   Fail          | Fail          Fail            Fail
+  ///   LEFT & TOP =  | Success    SoftFail   Fail
+  ///   --------------+-----------------------------
+  ///   Success       | Success    SoftFail   Fail
+  ///   SoftFail      | SoftFail   SoftFail   Fail
+  ///   Fail          | Fail           Fail   Fail
   ///
   /// An easy way of encoding this is as 0b11, 0b01, 0b00 for
   /// Success, SoftFail, Fail respectively.
diff --git a/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td b/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
index 455089588511f..cc1efd58005b0 100644
--- a/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
+++ b/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
@@ -71,14 +71,14 @@ def Inst3 : TestInstruction {
   let AsmString = "Inst3";
 }
 
-// CHECK-LABEL: DecodeStatus decodeFn_0(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_1(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_2(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_3(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: decodeToMCInst(unsigned Idx, DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
+// CHECK-LABEL: DecodeStatus decodeFn_0(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_1(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_2(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_3(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: decodeToMCInst(unsigned Idx, DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
 // CHECK: static constexpr DecodeFnTy decodeFnTable[]
 // CHECK-NEXT: decodeFn_0,
 // CHECK-NEXT: decodeFn_1,
 // CHECK-NEXT: decodeFn_2,
 // CHECK-NEXT: decodeFn_3,
-// CHECK: return decodeFnTable[Idx](S, insn, MI, Address, Decoder, DecodeComplete)
+// CHECK: return decodeFnTable[Idx](S, insn, MI, Address, Decoder)
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
index cdb1e327ad07d..98a78e29114c1 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
@@ -41,7 +41,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 15 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1,     // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK:       unsigned NumToSkip = *Ptr++;
 // CHECK-NEXT:  NumToSkip |= (*Ptr++) << 8;
@@ -54,7 +54,7 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 16 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1,    // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:       unsigned NumToSkip = *Ptr++;
 // CHECK-LARGE-NEXT:  NumToSkip |= (*Ptr++) << 8;
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
index 35657ff35c86f..be3bf812fb636 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
@@ -39,8 +39,8 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 19 */      OPC_CheckField, 3, 2, 0,
 // CHECK-NEXT: /* 23 */      OPC_TryDecode, {{[0-9]+}}, {{[0-9]+}}, 1,
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
-// CHECK: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 2, 1, 0,
 // CHECK-LARGE-NEXT: /* 4 */       OPC_CheckField, 5, 3, 0,
@@ -51,5 +51,5 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 24 */      OPC_TryDecode, {{[0-9]+}}, {{[0-9]+}}, 1,
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
-// CHECK-LARGE: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
index 4ac868dbb51aa..a46bd53cf1702 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
@@ -42,7 +42,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 15 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 4, 4, 0,
 // CHECK-LARGE-NEXT: /* 4 */       OPC_Scope, 8, 0, 0, // end scope at 16
@@ -51,4 +51,4 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 16 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
index ff1a7e33747ba..6be3af6b749aa 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
@@ -40,7 +40,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 17 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 250, 3, 4, 0,
@@ -50,4 +50,4 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 18 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp
index 9aa7d31c62693..acb2fc3b562ae 100644
--- a/llvm/utils/TableGen/DecoderEmitter.cpp
+++ b/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -865,7 +865,7 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
       "InsnType, uint64_t>;\n";
   StringRef DecodeParams =
       "DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const "
-      "MCDisassembler *Decoder, bool &DecodeComplete";
+      "MCDisassembler *Decoder";
 
   // Print the name of the decode function to OS.
   auto PrintDecodeFnName = [&OS, BucketBitWidth](unsigned DecodeIdx) {
@@ -904,7 +904,6 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
   OS << "// Handling " << Decoders.size() << " cases.\n";
   PrintTemplate();
   OS << "decodeToMCInst(unsigned Idx, " << DecodeParams << ") {\n";
-  OS << "  DecodeComplete = true;\n";
 
   if (UseFnTableInDecodeToMCInst) {
     // Build a table of function pointers
@@ -918,8 +917,7 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
     OS << "  };\n";
     OS << "  if (Idx >= " << Decoders.size() << ")\n";
     OS << "    llvm_unreachable(\"Invalid decoder index!\");\n";
-    OS << "  return decodeFnTable[Idx](S, insn, MI, Address, Decoder, "
-          "DecodeComplete);\n";
+    OS << "  return decodeFnTable[Idx](S, insn, MI, Address, Decoder);\n";
   } else {
     OS << "  " << TmpTypeDecl;
     OS << "  TmpType tmp;\n";
@@ -1052,9 +1050,7 @@ static void emitBinaryParser(raw_ostream &OS, indent Indent,
   StringRef Decoder = OpInfo.Decoder;
   if (!Decoder.empty()) {
     OS << Indent << "if (!Check(S, " << Decoder
-       << "(MI, tmp, Address, Decoder))) { "
-       << (OpInfo.HasCompleteDecoder ? "" : "DecodeComplete = false; ")
-       << "return MCDisassembler::Fail; }\n";
+       << "(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }\n";
   } else {
     OS << Indent << "MI.addOperand(MCOperand::createImm(tmp));\n";
   }
@@ -1069,9 +1065,7 @@ static std::string getDecoderString(const InstructionEncoding &Encoding) {
   StringRef DecoderMethod = Encoding.getDecoderMethod();
   if (!DecoderMethod.empty()) {
     OS << Indent << "if (!Check(S, " << DecoderMethod
-       << "(MI, insn, Address, Decoder))) { "
-       << (Encoding.hasCompleteDecoder() ? "" : "DecodeComplete = false; ")
-       << "return MCDisassembler::Fail; }\n";
+       << "(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }\n";
   } else {
     for (const OperandInfo &Op : Encoding.getOperands())
       emitBinaryParser(OS, Indent, Encoding, Op);
@@ -1682,15 +1676,13 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
       unsigned DecodeIdx = decodeULEB128AndIncUnsafe(Ptr);
 
       MI.clear();
-      MI.setOpcode(Opc);
-      bool DecodeComplete;)";
+      MI.setOpcode(Opc);)";
   if (IsVarLenInst) {
     OS << "\n      unsigned Len = InstrLenTable[Opc];\n"
        << "      makeUp(insn, Len);";
   }
   OS << R"(
-      S = decodeToMCInst(DecodeIdx, S, insn, MI, Address, DisAsm, DecodeComplete);
-      assert(DecodeComplete);
+      S = decodeToMCInst(DecodeIdx, S, insn, MI, Address, DisAsm);
 
       LLVM_DEBUG(dbgs() << Loc << ": OPC_Decode: opcode " << Opc
                    << ", using decoder " << DecodeIdx << ": "
@@ -1707,18 +1699,15 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
       // Perform the decode operation.
       MCInst TmpMI;
       TmpMI.setOpcode(Opc);
-      bool DecodeComplete;
-      S = decodeToMCInst(DecodeIdx, S, insn, TmpMI, Address, DisAsm, DecodeComplete);
+      S = decodeToMCInst(DecodeIdx, S, insn, TmpMI, Address, DisAsm);
       LLVM_DEBUG(dbgs() << Loc << ": OPC_TryDecode: opcode " << Opc
                    << ", using decoder " << DecodeIdx << ": ");
 
-      if (DecodeComplete) {
-        // Decoding complete.
-        LLVM_DEBUG(dbgs() << (S != MCDisassembler::Fail ? "PASS\n" : "FAIL\n"));
+      if (S != MCDisassembler::Fail) {
+        LLVM_DEBUG(dbgs() << (S != MCDisassembler::Success ? "Success\n" : "SoftFail\n"));
         MI = TmpMI;
         return S;
       }
-      assert(S == MCDisassembler::Fail);
       if (ScopeStack.empty()) {
         LLVM_DEBUG(dbgs() << "FAIL, returning FAIL\n");
         return MCDisassembler::Fail;

@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2025

@llvm/pr-subscribers-tablegen

Author: Rahul Joshi (jurahul)

Changes

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst. As a result, we can just use the return DecodeStatus from decodeToMCInst to decide whether to terminate the decoding and return or continue trying.

Also update the DecodeStatus combining table documentation to use the term "SoftFail" instead of "Unpredictable".

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

7 Files Affected:

  • (modified) llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h (+5-5)
  • (modified) llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td (+6-6)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission.td (+2-2)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td (+4-4)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td (+2-2)
  • (modified) llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td (+2-2)
  • (modified) llvm/utils/TableGen/DecoderEmitter.cpp (+9-20)
diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index cae2fbcac1fef..3d10380fa1d46 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -98,11 +98,11 @@ class LLVM_ABI MCDisassembler {
   /// from Success->SoftFail ->Fail can be done with a simple
   /// bitwise-AND:
   ///
-  ///   LEFT & TOP =  | Success       Unpredictable   Fail
-  ///   --------------+-----------------------------------
-  ///   Success       | Success       Unpredictable   Fail
-  ///   Unpredictable | Unpredictable Unpredictable   Fail
-  ///   Fail          | Fail          Fail            Fail
+  ///   LEFT & TOP =  | Success    SoftFail   Fail
+  ///   --------------+-----------------------------
+  ///   Success       | Success    SoftFail   Fail
+  ///   SoftFail      | SoftFail   SoftFail   Fail
+  ///   Fail          | Fail           Fail   Fail
   ///
   /// An easy way of encoding this is as 0b11, 0b01, 0b00 for
   /// Success, SoftFail, Fail respectively.
diff --git a/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td b/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
index 455089588511f..cc1efd58005b0 100644
--- a/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
+++ b/llvm/test/TableGen/DecoderEmitter/DecoderEmitterFnTable.td
@@ -71,14 +71,14 @@ def Inst3 : TestInstruction {
   let AsmString = "Inst3";
 }
 
-// CHECK-LABEL: DecodeStatus decodeFn_0(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_1(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_2(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: DecodeStatus decodeFn_3(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
-// CHECK-LABEL: decodeToMCInst(unsigned Idx, DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder, bool &DecodeComplete)
+// CHECK-LABEL: DecodeStatus decodeFn_0(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_1(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_2(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: DecodeStatus decodeFn_3(DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
+// CHECK-LABEL: decodeToMCInst(unsigned Idx, DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const MCDisassembler *Decoder)
 // CHECK: static constexpr DecodeFnTy decodeFnTable[]
 // CHECK-NEXT: decodeFn_0,
 // CHECK-NEXT: decodeFn_1,
 // CHECK-NEXT: decodeFn_2,
 // CHECK-NEXT: decodeFn_3,
-// CHECK: return decodeFnTable[Idx](S, insn, MI, Address, Decoder, DecodeComplete)
+// CHECK: return decodeFnTable[Idx](S, insn, MI, Address, Decoder)
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
index cdb1e327ad07d..98a78e29114c1 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission.td
@@ -41,7 +41,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 15 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1,     // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK:       unsigned NumToSkip = *Ptr++;
 // CHECK-NEXT:  NumToSkip |= (*Ptr++) << 8;
@@ -54,7 +54,7 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 16 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1,    // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:       unsigned NumToSkip = *Ptr++;
 // CHECK-LARGE-NEXT:  NumToSkip |= (*Ptr++) << 8;
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
index 35657ff35c86f..be3bf812fb636 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission2.td
@@ -39,8 +39,8 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 19 */      OPC_CheckField, 3, 2, 0,
 // CHECK-NEXT: /* 23 */      OPC_TryDecode, {{[0-9]+}}, {{[0-9]+}}, 1,
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
-// CHECK: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 2, 1, 0,
 // CHECK-LARGE-NEXT: /* 4 */       OPC_CheckField, 5, 3, 0,
@@ -51,5 +51,5 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 24 */      OPC_TryDecode, {{[0-9]+}}, {{[0-9]+}}, 1,
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
-// CHECK-LARGE: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstA(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
index 4ac868dbb51aa..a46bd53cf1702 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission3.td
@@ -42,7 +42,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 15 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 4, 4, 0,
 // CHECK-LARGE-NEXT: /* 4 */       OPC_Scope, 8, 0, 0, // end scope at 16
@@ -51,4 +51,4 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 16 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstBOp(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td b/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
index ff1a7e33747ba..6be3af6b749aa 100644
--- a/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
+++ b/llvm/test/TableGen/DecoderEmitter/trydecode-emission4.td
@@ -40,7 +40,7 @@ def InstB : TestInstruction {
 // CHECK-NEXT: /* 17 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-NEXT: };
 
-// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
 
 
 // CHECK-LARGE:      /* 0 */       OPC_CheckField, 250, 3, 4, 0,
@@ -50,4 +50,4 @@ def InstB : TestInstruction {
 // CHECK-LARGE-NEXT: /* 18 */      OPC_Decode, {{[0-9]+}}, {{[0-9]+}}, 1, // Opcode: InstA, DecodeIdx: 1
 // CHECK-LARGE-NEXT: };
 
-// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { DecodeComplete = false; return MCDisassembler::Fail; }
+// CHECK-LARGE: if (!Check(S, DecodeInstB(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }
diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp
index 9aa7d31c62693..acb2fc3b562ae 100644
--- a/llvm/utils/TableGen/DecoderEmitter.cpp
+++ b/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -865,7 +865,7 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
       "InsnType, uint64_t>;\n";
   StringRef DecodeParams =
       "DecodeStatus S, InsnType insn, MCInst &MI, uint64_t Address, const "
-      "MCDisassembler *Decoder, bool &DecodeComplete";
+      "MCDisassembler *Decoder";
 
   // Print the name of the decode function to OS.
   auto PrintDecodeFnName = [&OS, BucketBitWidth](unsigned DecodeIdx) {
@@ -904,7 +904,6 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
   OS << "// Handling " << Decoders.size() << " cases.\n";
   PrintTemplate();
   OS << "decodeToMCInst(unsigned Idx, " << DecodeParams << ") {\n";
-  OS << "  DecodeComplete = true;\n";
 
   if (UseFnTableInDecodeToMCInst) {
     // Build a table of function pointers
@@ -918,8 +917,7 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
     OS << "  };\n";
     OS << "  if (Idx >= " << Decoders.size() << ")\n";
     OS << "    llvm_unreachable(\"Invalid decoder index!\");\n";
-    OS << "  return decodeFnTable[Idx](S, insn, MI, Address, Decoder, "
-          "DecodeComplete);\n";
+    OS << "  return decodeFnTable[Idx](S, insn, MI, Address, Decoder);\n";
   } else {
     OS << "  " << TmpTypeDecl;
     OS << "  TmpType tmp;\n";
@@ -1052,9 +1050,7 @@ static void emitBinaryParser(raw_ostream &OS, indent Indent,
   StringRef Decoder = OpInfo.Decoder;
   if (!Decoder.empty()) {
     OS << Indent << "if (!Check(S, " << Decoder
-       << "(MI, tmp, Address, Decoder))) { "
-       << (OpInfo.HasCompleteDecoder ? "" : "DecodeComplete = false; ")
-       << "return MCDisassembler::Fail; }\n";
+       << "(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }\n";
   } else {
     OS << Indent << "MI.addOperand(MCOperand::createImm(tmp));\n";
   }
@@ -1069,9 +1065,7 @@ static std::string getDecoderString(const InstructionEncoding &Encoding) {
   StringRef DecoderMethod = Encoding.getDecoderMethod();
   if (!DecoderMethod.empty()) {
     OS << Indent << "if (!Check(S, " << DecoderMethod
-       << "(MI, insn, Address, Decoder))) { "
-       << (Encoding.hasCompleteDecoder() ? "" : "DecodeComplete = false; ")
-       << "return MCDisassembler::Fail; }\n";
+       << "(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }\n";
   } else {
     for (const OperandInfo &Op : Encoding.getOperands())
       emitBinaryParser(OS, Indent, Encoding, Op);
@@ -1682,15 +1676,13 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
       unsigned DecodeIdx = decodeULEB128AndIncUnsafe(Ptr);
 
       MI.clear();
-      MI.setOpcode(Opc);
-      bool DecodeComplete;)";
+      MI.setOpcode(Opc);)";
   if (IsVarLenInst) {
     OS << "\n      unsigned Len = InstrLenTable[Opc];\n"
        << "      makeUp(insn, Len);";
   }
   OS << R"(
-      S = decodeToMCInst(DecodeIdx, S, insn, MI, Address, DisAsm, DecodeComplete);
-      assert(DecodeComplete);
+      S = decodeToMCInst(DecodeIdx, S, insn, MI, Address, DisAsm);
 
       LLVM_DEBUG(dbgs() << Loc << ": OPC_Decode: opcode " << Opc
                    << ", using decoder " << DecodeIdx << ": "
@@ -1707,18 +1699,15 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
       // Perform the decode operation.
       MCInst TmpMI;
       TmpMI.setOpcode(Opc);
-      bool DecodeComplete;
-      S = decodeToMCInst(DecodeIdx, S, insn, TmpMI, Address, DisAsm, DecodeComplete);
+      S = decodeToMCInst(DecodeIdx, S, insn, TmpMI, Address, DisAsm);
       LLVM_DEBUG(dbgs() << Loc << ": OPC_TryDecode: opcode " << Opc
                    << ", using decoder " << DecodeIdx << ": ");
 
-      if (DecodeComplete) {
-        // Decoding complete.
-        LLVM_DEBUG(dbgs() << (S != MCDisassembler::Fail ? "PASS\n" : "FAIL\n"));
+      if (S != MCDisassembler::Fail) {
+        LLVM_DEBUG(dbgs() << (S != MCDisassembler::Success ? "Success\n" : "SoftFail\n"));
         MI = TmpMI;
         return S;
       }
-      assert(S == MCDisassembler::Fail);
       if (ScopeStack.empty()) {
         LLVM_DEBUG(dbgs() << "FAIL, returning FAIL\n");
         return MCDisassembler::Fail;

@jyknight
Copy link
Member

jyknight commented Sep 16, 2025

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst

But not when OpInfo.HasCompleteDecoder is false, right? So it seems like this change ought to break some tests...I'm surprised it doesn't.

@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst.

As I noted in the other PR, this is not exactly true. It only sets it to false when TryDecode fails. If Decode fails, it does not change the value of DecodeComplete and it remains true (initialized at the beginning of decodeToMCInst).

We should be careful here not to break downstream targets without providing a working replacement.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst

But not when OpInfo.HasCompleteDecoder is false, right? So it seems like this change ought to break some tests...I'm surprised it doesn't.

Right, I see that either the whole instruction decoder may be incomplete, or only certain operands may be incomplete, in which case other (non-incomplete) operands may fail to decode terminally vs the incomplete operands may bail to decode with an incomplete signal. Yeah, none of the tests seems to break through. Maybe AArch64 does not exercise this case?

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst.

As I noted in the other PR, this is not exactly true. It only sets it to false when TryDecode fails. If Decode fails, it does not change the value of DecodeComplete and it remains true (initialized at the beginning of decodeToMCInst).

We should be careful here not to break downstream targets without providing a working replacement.

Note, when there is any incomplete decode, we always generate TryDecode and never generate a Decode. So when hasIncompleteDecoder = true, its always TryDecode and there is no need to consider Decode AFAICT.

@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025 •
edited
Loading

Note, when there is any incomplete decode, we always generate TryDecode and never generate a Decode. So when hasIncompleteDecoder = true, its always TryDecode and there is no need to consider Decode AFAICT.

Right, but this doesn't make the description less confusing. The statement

Currently, the code generated by decoder emitter always set DeocdeComplete to false when returning Fail from decodeToMCInst.

is basically not true. There is a path that doesn't set DecodeComplete to false when returning Fail, and it is taken in 99% cases.

@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025 •
edited
Loading

In the end, I think there is no behavioral difference here.

Opc         Status  DecodeComplete  Result
Decode      Fail    true            return status
Decode      Success true            return status
TryDecode   Fail    false           leave scope
TryDecode   Success true            return status

We leave scope (or bail) iff TryDecode && DecodeComplete == false, which is equivalent to TryDecode && Status == Fail.

s-barannikov
s-barannikov reviewed Sep 16, 2025
View reviewed changes
llvm/utils/TableGen/DecoderEmitter.cpp
<< "(MI, insn, Address, Decoder))) { "
<< (Encoding.hasCompleteDecoder() ? "" : "DecodeComplete = false; ")
<< "return MCDisassembler::Fail; }\n";
<< "(MI, insn, Address, Decoder))) { return MCDisassembler::Fail; }\n";
Copy link
Contributor

@s-barannikov s-barannikov Sep 16, 2025

Choose a reason for hiding this comment

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

Drop the braces? They were needed before because there could be two statements inside them.
Same above.

s-barannikov
s-barannikov reviewed Sep 16, 2025
View reviewed changes
llvm/utils/TableGen/DecoderEmitter.cpp
// Decoding complete.
LLVM_DEBUG(dbgs() << (S != MCDisassembler::Fail ? "PASS\n" : "FAIL\n"));
if (S != MCDisassembler::Fail) {
LLVM_DEBUG(dbgs() << (S != MCDisassembler::Success ? "Success\n" : "SoftFail\n"));
Copy link
Contributor

@s-barannikov s-barannikov Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The condition is reversed.
Also, PASS/FAIL is printed by most other opcodes, I think we should stick to it. Or change them all to print Success/SoftFail/Fail (in a separate PR).

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025

In the end, I think there is no behavioral difference here.

Opc         Status  DecodeComplete  Result
Decode      Fail    true            return status
Decode      Success true            return status
TryDecode   Fail    false           leave scope
TryDecode   Success true            return status

We leave scope (or bail) iff TryDecode && DecodeComplete == false, which is equivalent to TryDecode && Status == Fail.

Wait, I thought this was a no-go based on @jyknight 's comment? in the sense, today, decodeToMCInst returns essentially one of these 3:

  1. decoding completed, and successful,
  2. decoding completed, unsuccessful (no further attempts necessary), and
  3. decoding incomplete (and unsuccessful)

Eliminating DecodeComplete would eliminate possibility 2 above.

@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025 •
edited
Loading

Eliminating DecodeComplete would eliminate possibility 2 above.

From inside decodeToMCInst, yes, it would be indistinguishable from 3. But this function is called from decodeInstruction, which has the opcode (Decode/TryDecode), which can help decide what to do next.

decoding completed, unsuccessful (no further attempts necessary), and

This is Opcode == Decode && Status == Fail in the table; we will pop scope or bail in this case.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025 •
edited
Loading

Eliminating DecodeComplete would eliminate possibility 2 above.

From inside decodeToMCInst, yes, it would be indistinguishable from 3. But this function is called from decodeInstruction, which has the opcode (Decode/TryDecode), which can help decide what to do next.

decoding completed, unsuccessful (no further attempts necessary), and

This is Opcode == Decode && Status == Fail in the table; we will pop scope or bail in this case.

Right, so for an instruction with say 2 operands, one has hasCompleteDecoder = true and other has hasCompleteDecoder = false, so that the overall instruction has hasCompleteDecoder=false and we emit TryDecode. Then if decoding of first operand fails, in current code, we return Fail and DecodeComplete = true, so we return with failure. In the new code, we will pop the scope since the function return fail, which is not the same as the old behavior. I think that was what @jyknight's scenario was where this PR would change the behavior

@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025

You are right. I confused hasCompleteDecoder flag on an instruction and on operands. It is the operands' flag that makes difference; instruction's flag only affects the chosen opcode.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025

Thanks. I'll close this. I am trying to work on some scheme to help resolve conflicts without using decoder namespaces. Will follow up on discourse or the other PR.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025

Closing.

@jurahul jurahul closed this Sep 16, 2025
@s-barannikov
Copy link
Contributor

s-barannikov commented Sep 16, 2025 •
edited
Loading

Now I think that Decode is redundant :)

  1. In Decode case, decodeToMCInst always returns DecodeComplete set to true. It is asserted, so I can't be wrong here.
  2. If decodeToMCInst returns DecodeComplete set to true, TryDecode behaves exactly like Decode -- returns the decoded instruction and the status.

So we could always emit TryDecode. The cost is an additional check for DecodeComplete, which should be tiny compared to the cost of a decoder function.

Is my logic flawed?

@jurahul
Copy link
Contributor Author

jurahul commented Sep 16, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@s-barannikov s-barannikov s-barannikov left review comments

@topperc topperc Awaiting requested review from topperc

@jyknight jyknight Awaiting requested review from jyknight

Assignees

No one assigned

Labels

llvm:mc Machine (object) code tablegen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jurahul @llvmbot @jyknight @s-barannikov