[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

[Prabhuk]

I am investigating a failure when I compile a cortex-m33 target with ToT llvm: "tBcc: expected 3 operands, got 5"

This change seems to be the root cause of the problem. Any help in investigating this will be highly appreciated @s-barannikov

[s-barannikov]

Sorry to hear that.
tBcc has three operands, a branch target and a (two-operand) predicate.
My guess is the predicate is added twice: first by the generated decoding function, and then by AddThumbPredicate. This function has a special case for tBcc that should avoid adding the predicate the second time

      if (ITBlock.instrInITBlock())
        S = SoftFail;
      else
        return Success;

but it looks like it can still add it if the branch is in an IT block. Is that the case?

Are you disassembling a third-party binary or a just built one? If it is the latter, a source assembly snippet would be very helpful.

[s-barannikov]

Can you check if #156540 fixes the issue? patch link

[Prabhuk]

Can you check if #156540 fixes the issue? patch link

This was reported by a customer. I've shared a llvm-objdump binary with your fix with them to try and report back.

[Prabhuk]

Can you check if #156540 fixes the issue? patch link

This was reported by a customer. I've shared a llvm-objdump binary with your fix with them to try and report back.

We are not able to repro this failure anymore. I am not sure if this was a flake or not. We will keep an eye out for this failure and will try your patch if we can reproduce the failure again. Thanks for responding right away.