[ARM] Accept ldrb.w mnemonic for certain addressing modes (PR43382)

[chill]

Summary

Encoding T3 of ldrb in A7.7.46 (Armv7-M ARM Revidion E.d) offset allows the ".w" mnemonic suffix, even
though the preferred disassembly is without the suffix.
We did not accept the suffix; this patch fixes it by adding a few instruction aliases.

Is there a less hackish way of doing it ?

[efriedma]

Adding aliases seems like the right approach; we have aliases for a bunch of other instructions.

I'm not sure why you need the AsmMatchConverter, though; needs a comment to explain.

llvm/lib/Target/ARM/ARMInstrThumb2.td

1536

Why is Rt listed twice here?

[chill]

In D68916#1708356, @efriedma wrote:

Adding aliases seems like the right approach; we have aliases for a bunch of other instructions.

I'm not sure why you need the AsmMatchConverter, though; needs a comment to explain.

Why is Rt listed twice here?

For both aliases we need something to match the writeback out operand of t2LDRB_PRE and t2LDRB_POST.
For t2LDR_PRE the writeback is well hidden in $addr operand and something like $addr.base does not seem to work.
For t2LDR_POST the $Rn operand is not a GPR, it's a memory operand and likewise does not match.

$Rt is just a placeholder in both cases.

And that's why we need the AsmMatchConverters - to replace *that* Rt with the correct writeback register from the memory operands.

[efriedma]

Oh, that makes sense. But needs a comment on the pattern, and in cvtThumb2LDRB_PRE/POST.

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

Do other PRE/POST load/store instructions have the same issue?

[sdesmalen]

llvm/lib/Target/ARM/ARMInstrThumb2.td

1404

Does LDRH (and maybe other instructions) not suffer the same issue? If so, should these InstAliases be moved into T2I_ld?

[chill]

llvm/lib/Target/ARM/ARMInstrThumb2.td

1404

Yes, other instructions have the same issue. I plan to fix (some of) them in separate patches, once the approach with AsmMatchConverter is validated as good enough. Then I'll look for opportunities to refactor.

[chill]

Ping?

[chill]

Added some comments.

[efriedma]

Do you have a reply to the following?

In D68916#1708799, @efriedma wrote:

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

[sdesmalen]

In D68916#1741305, @efriedma wrote:

Do you have a reply to the following?

In D68916#1708799, @efriedma wrote:

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

From my discussion with @chill today, that seems to be the issue indeed. TableGen doesn't use the $addr.base = $Rn_wb constraint from the instruction to materialize the second output register for the InstAliases. This is probably best fixed in TableGen itself, but having seen some of that code I can appreciate there is a tradeoff here whether or not to go the AsmMatchConverter route.

[nickdesaulniers]

In D68916#1741416, @sdesmalen wrote:

TableGen doesn't use the $addr.base = $Rn_wb constraint from the instruction to materialize the second output register for the InstAliases.

Heh, I observed the same limitation. I was able to partially work around it with t2AsmPseudo and custom parsing logic in D96632.

llvm/lib/Target/ARM/ARMInstrThumb2.td

 let canFoldAsLoad = 1, isReMaterializable = 1  in
 defm t2LDR   : T2I_ld<0, 0b10, "ldr", IIC_iLoad_i, IIC_iLoad_si, GPR, load>;
 
 // Loads with zero extension
 defm t2LDRH  : T2I_ld<0, 0b01, "ldrh", IIC_iLoad_bh_i, IIC_iLoad_bh_si,
                       GPRnopc, zextloadi16>;
 defm t2LDRB  : T2I_ld<0, 0b00, "ldrb", IIC_iLoad_bh_i, IIC_iLoad_bh_si,
                       GPRnopc, zextloadi8>;
+def : t2InstAlias<"ldrb${p}.w $Rt, $addr", (t2LDRBi8 GPRnopc:$Rt, t2addrmode_negimm8:$addr,
+                                            pred:$p)>;
 
 // Loads with sign extension
 defm t2LDRSH : T2I_ld<1, 0b01, "ldrsh", IIC_iLoad_bh_i, IIC_iLoad_bh_si,
                       GPRnopc, sextloadi16>;
 defm t2LDRSB : T2I_ld<1, 0b00, "ldrsb", IIC_iLoad_bh_i, IIC_iLoad_bh_si,
                       GPRnopc, sextloadi8>;
 
 let mayLoad = 1, hasSideEffects = 0, hasExtraDefRegAllocReq = 1 in {
def t2LDR_POST : T2Ipostldst<0, 0b10, 1, 0, (outs GPR:$Rt, GPR:$Rn_wb),
                           AddrModeT2_i8, IndexModePost, IIC_iLoad_iu,
                           "ldr", "\t$Rt, $Rn$offset", "$Rn = $Rn_wb", []>,
                   Sched<[WriteLd]>;
 
 def t2LDRB_PRE : T2Ipreldst<0, 0b00, 1, 1, (outs GPR:$Rt, GPR:$Rn_wb),
                             (ins t2addrmode_imm8_pre:$addr),
                             AddrModeT2_i8, IndexModePre, IIC_iLoad_bh_iu,
                             "ldrb", "\t$Rt, $addr!", "$addr.base = $Rn_wb", []>,
-                 Sched<[WriteLd]>;
+                 Sched<[WriteLd]> {
+  let AsmMatchConverter = "cvtThumb2LDRB_PRE";
+}
 
 def t2LDRB_POST : T2Ipostldst<0, 0b00, 1, 0, (outs GPR:$Rt, GPR:$Rn_wb),
                           (ins addr_offset_none:$Rn, t2am_imm8_offset:$offset),
                           AddrModeT2_i8, IndexModePost, IIC_iLoad_bh_iu,
                           "ldrb", "\t$Rt, $Rn$offset", "$Rn = $Rn_wb", []>,
-                  Sched<[WriteLd]>;
+                  Sched<[WriteLd]> {
+  let AsmMatchConverter = "cvtThumb2LDRB_POST";
+}
 
 def t2LDRH_PRE : T2Ipreldst<0, 0b01, 1, 1, (outs GPR:$Rt, GPR:$Rn_wb),
                             (ins t2addrmode_imm8_pre:$addr),
                             AddrModeT2_i8, IndexModePre, IIC_iLoad_bh_iu,
                             "ldrh", "\t$Rt, $addr!", "$addr.base = $Rn_wb", []>,
                 Sched<[WriteLd]>;
 
 def t2LDRH_POST : T2Ipostldst<0, 0b01, 1, 0, (outs GPR:$Rt, GPR:$Rn_wb),

 
 def t2LDRSH_POST : T2Ipostldst<1, 0b01, 1, 0, (outs GPR:$Rt, GPR:$Rn_wb),
                           (ins addr_offset_none:$Rn, t2am_imm8_offset:$offset),
                           AddrModeT2_i8, IndexModePost, IIC_iLoad_bh_iu,
                           "ldrsh", "\t$Rt, $Rn$offset", "$Rn = $Rn_wb", []>,
                   Sched<[WriteLd]>;
 } // mayLoad = 1, hasSideEffects = 0
 
+// In the following aliases we need something to match the writeback out
+// operand of t2LDRB_PRE and t2LDRB_POST. For t2LDR_PRE the writeback is well
+// hidden in $addr operand, for t2LDR_POST the $Rn operand is not a GPR,
+// it's a memory operand.
+// In both case $Rt is just a placeholder and we have an AsmMatchCoverter
+// to replace that $Rt with the correct writeback register, extracted from
+// the memory operand.
+def : t2InstAlias<
+        "ldrb${p}.w $Rt, $addr!",
+        (t2LDRB_PRE GPR:$Rt, GPR:$Rt, t2addrmode_imm8_pre:$addr, pred:$p)>;
+
+def : t2InstAlias<
+        "ldrb${p}.w $Rt, $Rn$offset",
+        (t2LDRB_POST GPR:$Rt, GPR:$Rt, addr_offset_none:$Rn,
+                     t2am_imm8_offset:$offset, pred:$p)>;
+
 // LDRT, LDRBT, LDRHT, LDRSBT, LDRSHT all have offset mode (PUW=0b110).
 // Ref: A8.6.57 LDR (immediate, Thumb) Encoding T4
 class T2IldT<bit signed, bits<2> type, string opc, InstrItinClass ii>
   : T2Ii8<(outs rGPR:$Rt), (ins t2addrmode_posimm8:$addr), ii, opc,
           "\t$Rt, $addr", []>, Sched<[WriteLd]> {
   bits<4> Rt;
   bits<13> addr;
   let Inst{31-27} = 0b11111;

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

#include "ARMGenAsmMatcher.inc"
   OperandMatchResultTy parseVectorList(OperandVector &);
   OperandMatchResultTy parseVectorLane(VectorLaneTy &LaneKind, unsigned &Index,
                                        SMLoc &EndLoc);
 
   // Asm Match Converter Methods
   void cvtThumbMultiply(MCInst &Inst, const OperandVector &);
   void cvtThumbBranches(MCInst &Inst, const OperandVector &);
   void cvtMVEVMOVQtoDReg(MCInst &Inst, const OperandVector &);
+  void cvtThumb2LDRB_PRE(MCInst &Inst, const OperandVector &);
+  void cvtThumb2LDRB_POST(MCInst &Inst, const OperandVector &);
 
   bool validateInstruction(MCInst &Inst, const OperandVector &Ops);
   bool processInstruction(MCInst &Inst, const OperandVector &Ops, MCStreamer &Out);
   bool shouldOmitCCOutOperand(StringRef Mnemonic, OperandVector &Operands);
   bool shouldOmitPredicateOperand(StringRef Mnemonic, OperandVector &Operands);
   bool shouldOmitVectorPredicateOperand(StringRef Mnemonic, OperandVector &Operands);
   bool isITBlockTerminator(MCInst &Inst) const;
   void fixupGNULDRDAlias(StringRef Mnemonic, OperandVector &Operands);
void ARMAsmParser::cvtMVEVMOVQtoDReg(
   ((ARMOperand &)*Operands[3]).addRegOperands(Inst, 1); // Rt2
   ((ARMOperand &)*Operands[4]).addRegOperands(Inst, 1); // Qd
   ((ARMOperand &)*Operands[5]).addMVEPairVectorIndexOperands(Inst, 1); // idx
   // skip second copy of Qd in Operands[6]
   ((ARMOperand &)*Operands[7]).addMVEPairVectorIndexOperands(Inst, 1); // idx2
   ((ARMOperand &)*Operands[1]).addCondCodeOperands(Inst, 2); // condition code
 }
 
+// Aliases of t2LRDB_PRE/POST currently have no way of specifying the writeback
+// register, so they use the destination register as a placeholder.
+// These converters replace the writeback operand with the correct
+// register from the memory operand.
+void ARMAsmParser::cvtThumb2LDRB_PRE(MCInst &Inst,
+                                     const OperandVector &Operands) {
+  assert(Operands.size() == 5 || Operands.size() == 6); // ldrb vs. ldrb.w
+  const int N = Operands.size() == 5 ? 2 : 3;
+  ((ARMOperand &)*Operands[N]).addRegOperands(Inst, 1);             // Rt
+  ((ARMOperand &)*Operands[N + 1]).addMemNoOffsetOperands(Inst, 1); // Rn_wb
+  ((ARMOperand &)*Operands[N + 1])
+      .addMemImmOffsetOperands(Inst, 2); // addr == Rn +/- imm
+  ((ARMOperand &)*Operands[1]).addCondCodeOperands(Inst, 2); // condition code
+}
+
+void ARMAsmParser::cvtThumb2LDRB_POST(MCInst &Inst,
+                                     const OperandVector &Operands) {
+  assert(Operands.size() == 5 || Operands.size() == 6); // ldrb vs. ldrb.w
+  const int N = Operands.size() == 5 ? 2 : 3;
+  ((ARMOperand &)*Operands[N]).addRegOperands(Inst, 1);             // Rt
+  ((ARMOperand &)*Operands[N + 1]).addMemNoOffsetOperands(Inst, 1); // Rn_wb
+  ((ARMOperand &)*Operands[N + 1]).addMemNoOffsetOperands(Inst, 1); // Rn
+  ((ARMOperand &)*Operands[N + 2]).addImmOperands(Inst, 1); // offset
+  ((ARMOperand &)*Operands[1]).addCondCodeOperands(Inst, 2); // condition code
+}
+
 /// Parse an ARM memory expression, return false if successful else return true
 /// or an error.  The first token must be a '[' when called.
 bool ARMAsmParser::parseMemory(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   SMLoc S, E;
   if (Parser.getTok().isNot(AsmToken::LBrac))
     return TokError("Token is not a Left Bracket");
   S = Parser.getTok().getLoc();

llvm/test/MC/ARM/basic-thumb2-instructions.s

 @ CHECK: ldr.w  r7, [sp, r2, lsl #1]    @ encoding: [0x5d,0xf8,0x12,0x70]
 @ CHECK: ldr.w  r7, [sp, r2]            @ encoding: [0x5d,0xf8,0x02,0x70]
 
 
 @------------------------------------------------------------------------------
 @ LDRB(immediate)
 @------------------------------------------------------------------------------
         ldrb r5, [r5, #-4]
+        ldrb.w r5, [r5, #-4]
         ldrb r5, [r6, #32]
+        ldrb.w r5, [r6, #32]
         ldrb r5, [r6, #33]
         ldrb r5, [r6, #257]
         ldrb.w lr, [r7, #257]
         ldrb r5, [r8, #255]!
+        ldrb.w r5, [r8, #255]!
         ldrb r2, [r5, #4]!
         ldrb r1, [r4, #-4]!
         ldrb lr, [r3], #255
         ldrb r9, [r2], #4
+        ldrb.w r9, [r2], #4
         ldrb r3, [sp], #-4
 
 @ CHECK: ldrb  r5, [r5, #-4]           @ encoding: [0x15,0xf8,0x04,0x5c]
+@ CHECK: ldrb  r5, [r5, #-4]           @ encoding: [0x15,0xf8,0x04,0x5c]
+@ CHECK: ldrb.w  r5, [r6, #32]           @ encoding: [0x96,0xf8,0x20,0x50]
 @ CHECK: ldrb.w  r5, [r6, #32]           @ encoding: [0x96,0xf8,0x20,0x50]
 @ CHECK: ldrb.w  r5, [r6, #33]           @ encoding: [0x96,0xf8,0x21,0x50]
 @ CHECK: ldrb.w  r5, [r6, #257]          @ encoding: [0x96,0xf8,0x01,0x51]
 @ CHECK: ldrb.w  lr, [r7, #257]          @ encoding: [0x97,0xf8,0x01,0xe1]
 @ CHECK: ldrb  r5, [r8, #255]!         @ encoding: [0x18,0xf8,0xff,0x5f]
+@ CHECK: ldrb  r5, [r8, #255]!         @ encoding: [0x18,0xf8,0xff,0x5f]
 @ CHECK: ldrb  r2, [r5, #4]!           @ encoding: [0x15,0xf8,0x04,0x2f]
 @ CHECK: ldrb  r1, [r4, #-4]!          @ encoding: [0x14,0xf8,0x04,0x1d]
 @ CHECK: ldrb  lr, [r3], #255          @ encoding: [0x13,0xf8,0xff,0xeb]
 @ CHECK: ldrb  r9, [r2], #4            @ encoding: [0x12,0xf8,0x04,0x9b]
+@ CHECK: ldrb  r9, [r2], #4            @ encoding: [0x12,0xf8,0x04,0x9b]
 @ CHECK: ldrb  r3, [sp], #-4           @ encoding: [0x1d,0xf8,0x04,0x39]
 
 
 @------------------------------------------------------------------------------
 @ LDRB(register)
 @------------------------------------------------------------------------------
         ldrb r1, [r8, r1]
         ldrb.w r4, [r5, r2]