[RISCV][NFC] Simplify decoding code of disassembler

[• pcwang-thead]

Summary

The decoding parts are reduplicative, we add a macro to simplify
the code.

[• pcwang-thead]

clang-format

[• pcwang-thead]

clang-format

[• pcwang-thead]

Remove extra space and colon.

[kito-cheng]

LGTM, great simplify!

[• pcwang-thead]

Rename DBG_MSG to DESC.

[kito-cheng]

still lgtm

[jrtc27]

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

493

Typo: FEATURE everywhere

[• pcwang-thead]

Fix typo.

[craig.topper]

The order we search these tables doesn't make sense. Shouldn't we be checking the standard table first, then falling back to vendor tables after that? Even with a vendor extension enabled, the majority of the instructions are going to be standard so we should decode them first.

[jrtc27]

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

[• pcwang-thead]

In D151309#4370636, @craig.topper wrote:

The order we search these tables doesn't make sense. Shouldn't we be checking the standard table first, then falling back to vendor tables after that? Even with a vendor extension enabled, the majority of the instructions are going to be standard so we should decode them first.

I didn't change the order, it is the same. Does the order really matter? The vendor extensions must not conflict with standard encoding space.
But from the perspective of compile time, I think the order may matter because for most instructions they can exit the decoding early.

[• pcwang-thead]

In D151309#4370646, @jrtc27 wrote:

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

Does the replacement happen in decoding not encoding? Just curious about the functionality of such replacement.

[jrtc27]

In D151309#4370683, @pcwang-thead wrote:

In D151309#4370646, @jrtc27 wrote:

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

Does the replacement happen in decoding not encoding? Just curious about the functionality of such replacement.

Both directions need to be consistent...

[craig.topper]

In D151309#4370646, @jrtc27 wrote:

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

Well my thought doesn't work anyway because Zcmp and Zcmt are in a separate decoder namespace but the instructions they replace from C aren't in a namespace. So they have to be checked first. Same with Zfinx/Zdinx vs F/D.

[craig.topper]

In D151309#4370756, @craig.topper wrote:

In D151309#4370646, @jrtc27 wrote:

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

Well my thought doesn't work anyway because Zcmp and Zcmt are in a separate decoder namespace but the instructions they replace from C aren't in a namespace. So they have to be checked first. Same with Zfinx/Zdinx vs F/D.

Or maybe the predicate check in the DecoderTable fixes that?

Anyway it doesn't affect this patch just an observation to think about.

[jrtc27]

In D151309#4370756, @craig.topper wrote:

In D151309#4370646, @jrtc27 wrote:

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

Well my thought doesn't work anyway because Zcmp and Zcmt are in a separate decoder namespace but the instructions they replace from C aren't in a namespace. So they have to be checked first. Same with Zfinx/Zdinx vs F/D.

Hm, actually, shouldn't the predicates on the instructions themselves make it all work, both our case downstream and the Zcm*+Z*inx cases?

[• pcwang-thead]

Simplify decoding with addtional operation (e.g. add SP operand).

[• pcwang-thead]

@jrtc27 @craig.topper Is there any problem here?

[craig.topper]

LGTM

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
                                                ArrayRef<uint8_t> Bytes,
                                                uint64_t Address,
                                                raw_ostream &CS) const {
   // TODO: This will need modification when supporting instruction set
   // extensions with instructions > 32-bits (up to 176 bits wide).
   uint32_t Insn;
   DecodeStatus Result;
 
+#define TRY_TO_DECODE(FEATURE_CHECKS, DECODER_TABLE, DESC)                     \
+  do {                                                                         \
+    if (FEATURE_CHECKS) {                                                      \
+      LLVM_DEBUG(dbgs() << "Trying " DESC ":\n");                              \
+      Result = decodeInstruction(DECODER_TABLE, MI, Insn, Address, this, STI); \
+      if (Result != MCDisassembler::Fail)                                      \
+        return Result;                                                         \
+    }                                                                          \
+  } while (false)
+
+#define TRY_TO_DECODE_FEATURE(FEATURE, DECODER_TABLE, DESC)                    \
+  TRY_TO_DECODE(STI.hasFeature(FEATURE), DECODER_TABLE, DESC)
+
   // It's a 32 bit instruction if bit 0 and 1 are 1.
   if ((Bytes[0] & 0x3) == 0x3) {
     if (Bytes.size() < 4) {
       Size = 0;
       return MCDisassembler::Fail;
     }
     Size = 4;
 
     Insn = support::endian::read32le(Bytes.data());
 
-    if (STI.hasFeature(RISCV::FeatureStdExtZdinx) &&
-        !STI.hasFeature(RISCV::Feature64Bit)) {
-      LLVM_DEBUG(dbgs() << "Trying RV32Zdinx table (Double in Integer and"
-                           "rv32)\n");
-      Result = decodeInstruction(DecoderTableRV32Zdinx32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureStdExtZfinx)) {
-      LLVM_DEBUG(dbgs() << "Trying RVZfinx table (Float in Integer):\n");
-      Result = decodeInstruction(DecoderTableRVZfinx32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXVentanaCondOps)) {
-      LLVM_DEBUG(dbgs() << "Trying Ventana custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableVentana32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadBa)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadBa custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadBa32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadBb)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadBb custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadBb32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadBs)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadBs custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadBs32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadCondMov)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadCondMov custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadCondMov32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadCmo)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadCmo custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadCmo32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadFMemIdx)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadFMemIdx custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadFMemIdx32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadMac)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadMac custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadMac32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadMemIdx)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadMemIdx custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadMemIdx32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadMemPair)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadMemPair custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadMemPair32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadSync)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadSync custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableTHeadSync32, MI, Insn, Address,
-                                 this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXTHeadVdot)) {
-      LLVM_DEBUG(dbgs() << "Trying XTHeadVdot custom opcode table:\n");
-      Result =
-          decodeInstruction(DecoderTableTHeadV32, MI, Insn, Address, this, STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
-    if (STI.hasFeature(RISCV::FeatureVendorXSfvcp)) {
-      LLVM_DEBUG(dbgs() << "Trying SiFive VCIX custom opcode table:\n");
-      Result = decodeInstruction(DecoderTableXSfvcp32, MI, Insn, Address, this,
-                                 STI);
-      if (Result != MCDisassembler::Fail)
-        return Result;
-    }
+    TRY_TO_DECODE(STI.hasFeature(RISCV::FeatureStdExtZdinx) &&
+                      !STI.hasFeature(RISCV::Feature64Bit),
+                  DecoderTableRV32Zdinx32,
+                  "RV32Zdinx table (Double in Integer and rv32)");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZfinx, DecoderTableRVZfinx32,
+                          "RVZfinx table (Float in Integer)");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXVentanaCondOps,
+                          DecoderTableVentana32, "Ventana custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadBa, DecoderTableTHeadBa32,
+                          "XTHeadBa custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadBb, DecoderTableTHeadBb32,
+                          "XTHeadBb custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadBs, DecoderTableTHeadBs32,
+                          "XTHeadBs custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadCondMov,
+                          DecoderTableTHeadCondMov32,
+                          "XTHeadCondMov custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadCmo, DecoderTableTHeadCmo32,
+                          "XTHeadCmo custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadFMemIdx,
+                          DecoderTableTHeadFMemIdx32,
+                          "XTHeadFMemIdx custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadMac, DecoderTableTHeadMac32,
+                          "XTHeadMac custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadMemIdx,
+                          DecoderTableTHeadMemIdx32,
+                          "XTHeadMemIdx custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadMemPair,
+                          DecoderTableTHeadMemPair32,
+                          "XTHeadMemPair custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadSync,
+                          DecoderTableTHeadSync32,
+                          "XTHeadSync custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadVdot, DecoderTableTHeadV32,
+                          "XTHeadVdot custom opcode table");
+    TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXSfvcp, DecoderTableXSfvcp32,
+                          "SiFive VCIX custom opcode table");
 
-    LLVM_DEBUG(dbgs() << "Trying RISCV32 table :\n");
+    LLVM_DEBUG(dbgs() << "Trying RISCV32 table:\n");
     return decodeInstruction(DecoderTable32, MI, Insn, Address, this, STI);
   }
 
   if (Bytes.size() < 2) {
     Size = 0;
     return MCDisassembler::Fail;
   }
   Size = 2;
 
   Insn = support::endian::read16le(Bytes.data());
 
   if (!STI.hasFeature(RISCV::Feature64Bit)) {
     LLVM_DEBUG(
         dbgs() << "Trying RISCV32Only_16 table (16-bit Instruction):\n");
     // Calling the auto-generated decoder function.
     Result = decodeInstruction(DecoderTableRISCV32Only_16, MI, Insn, Address,
                                this, STI);
     if (Result != MCDisassembler::Fail) {
       addSPOperands(MI);
       return Result;
     }
   }
-  if (STI.hasFeature(RISCV::FeatureStdExtZcmt)) {
-    LLVM_DEBUG(
-        dbgs() << "Trying Zcmt table (16-bit Table Jump Instructions):\n");
-    Result = decodeInstruction(DecoderTableRVZcmt16, MI, Insn, Address,
-                               this, STI);
-    if (Result != MCDisassembler::Fail)
-      return Result;
-  }
-  if (STI.hasFeature(RISCV::FeatureStdExtZcmp)) {
-    LLVM_DEBUG(
-        dbgs()
-        << "Trying Zcmp table (16-bit Push/Pop & Double Move Instructions):\n");
-    Result =
-        decodeInstruction(DecoderTableRVZcmp16, MI, Insn, Address, this, STI);
-    if (Result != MCDisassembler::Fail)
-      return Result;
-  }
+
+  TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZcmt, DecoderTableRVZcmt16,
+                        "Zcmt table (16-bit Table Jump Instructions)");
+  TRY_TO_DECODE_FEATURE(
+      RISCV::FeatureStdExtZcmp, DecoderTableRVZcmp16,
+      "Zcmp table (16-bit Push/Pop & Double Move Instructions)");
 
   LLVM_DEBUG(dbgs() << "Trying RISCV_C table (16-bit Instruction):\n");
   // Calling the auto-generated decoder function.
   Result = decodeInstruction(DecoderTable16, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail)
     addSPOperands(MI);
 
   return Result;
 }