[ARM] Verify that disassembled instruction is correct

[s-barannikov]

This change adds basic MCInst verification (checks the number of operands) and fixes detected bugs.

• RFE* instructions have only one operand, but DecodeRFEInstruction added two.
• DecodeMVEModImmInstruction and DecodeMVEVCMP added a vpred operand, but this is what AddThumbPredicate normally does. This resulted in an extra vpred operand.
• DecodeMVEVADCInstruction added an extra immediate operand.
• getARMInstruction added a pred operand to instructions that don't have one (via DecodePredicateOperand).
• AddThumb1SBit appended an extra register operand to instructions that don't modify CPSR (such as tBL).
• Instructions in NEONDup namespace have pred operand that the generated code successfully decodes. The operand was added once again by getARMInstruction/getThumbInstruction via AddThumbPredicate.

Functional changes extracted from #156540.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: Sergei Barannikov (s-barannikov)

Changes

RFE instructions have only one operand, but the decoder was adding two.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp (-18)
• (modified) llvm/test/MC/Disassembler/ARM/arm-tests.txt (+1-1)

diff --git a/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp b/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
index 41d554f2cece9..4c92490206d93 100644
--- a/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
+++ b/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
@@ -1365,24 +1365,6 @@ static DecodeStatus DecodeRFEInstruction(MCInst &Inst, unsigned Insn,
   DecodeStatus S = MCDisassembler::Success;
 
   unsigned Rn = fieldFromInstruction(Insn, 16, 4);
-  unsigned mode = fieldFromInstruction(Insn, 23, 2);
-
-  switch (mode) {
-    case 0:
-      mode = ARM_AM::da;
-      break;
-    case 1:
-      mode = ARM_AM::ia;
-      break;
-    case 2:
-      mode = ARM_AM::db;
-      break;
-    case 3:
-      mode = ARM_AM::ib;
-      break;
-  }
-
-  Inst.addOperand(MCOperand::createImm(mode));
   if (!Check(S, DecodeGPRRegisterClass(Inst, Rn, Address, Decoder)))
     return MCDisassembler::Fail;
 
diff --git a/llvm/test/MC/Disassembler/ARM/arm-tests.txt b/llvm/test/MC/Disassembler/ARM/arm-tests.txt
index 008bb1154e57f..a1016cdb5c8cc 100644
--- a/llvm/test/MC/Disassembler/ARM/arm-tests.txt
+++ b/llvm/test/MC/Disassembler/ARM/arm-tests.txt
@@ -354,7 +354,7 @@
 # CHECK:         strheq  r0, [r0, -r0]
 0xb0 0x00 0x00 0x01
 
-# CHECK: rfedb	#4!
+# CHECK: rfedb	r2!
 0x14 0x0 0x32 0xf9
 
 # CHECK: stc2l	p0, c0, [r2], #-96

[s-barannikov]

I'm not sure about this change.
AddThumbPredicate not only adds a predicate operand, but also advances IT/VPT block states.
We don't need to add the operand here (it is added by the generated decoder), but should we advance the states? Can these instructions appear in IT block?

Also, DecodePredicateOperand that adds the operand doesn't take into account IT state. Should it?

Note that we don't call this method for instructions in VFPV8 namespace above and v8Crypto/v8NEON below.

The tests magically pass, but that doesn't give me 100% confidence in this change.

[s-barannikov]

I think I've figure it out. I should call UpdateThumbVFPPredicate here instead.
Not sure about other tables I mentioned, but this should preserve the existing behavior for NEONDup.

[s-barannikov]

Not sure if this should be a fatal error. Maybe report an error and continue? In case there are bugs not detected by tests.

[s-barannikov]

gentle ping

[davemgreen]

I think this is OK. LGTM

[s-barannikov]

Thanks