[PATCH] D96632: [THUMB2] add .w suffixes for ldr/str w/ immediates

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:1568
+// .w suffixes; t2InstAlias isn't descriptive enough for $Rn = $Rn_wb
+// Constraint.
+def t2LDR_PRE_imm : t2AsmPseudo<"ldr${p}.w $Rt, $addr!",
----------------
DavidSpickett wrote:
> Do you mean "$Rn != $Rn_wb". I see what you mean but out of the context of the armarm, != makes more immediate sense.
No; that's what's above for t2LDR_POST (I guess it should be `"$addr.base = $Rn_wb"` for PRE in my added comment).  I tried wrapping these in `let` blocks while using `t2InstAlias` to describe the "Constraint"s but it seems I may not be the first to have hit that: https://reviews.llvm.org/D68916#1741416.

I've updated the comment.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7672
+                   "destination register and base register can't be identical");
+    if (Inst.getOpcode() == ARM::t2LDR_POST_imm ||
+        Inst.getOpcode() == ARM::t2STR_POST_imm) {
----------------
DavidSpickett wrote:
> You'll add a check here for PRE too, or is that not needed for some reason?
> 
> This would use the imm12 range from the T3 encoding I think.
I'm not really sure if operands have additional validation, but I suspect that the use of `t2addrmode_imm8_pre` for PRE is validated elsewhere; and perhaps `t2am_imm8_offset` used for POST is not.  If I add checks for pre here and feed in bad input, it wont matter, because we never get this far. I suspect the operand validation is occurring before instruction matching.

For example, consider:

    ldr.w r3, [r1, #4095]!

(so T4 pre increment w/ writeback, only accepts immediates in the range [-255,255]).  With this patch applied:

```
error: invalid instruction, any one of the following would fix this:
ldr.w r3, [r1, #4095]!
^
foo.s:17:11: note: invalid operand for instruction
ldr.w r3, [r1, #4095]!
          ^
foo.s:17:22: note: too many operands for instruction
ldr.w r3, [r1, #4095]!
                     ^
```

which looks to me like the instruction is not getting matched, BEFORE any of this added parsing logic.  Perhaps the error text for `t2addrmode_imm8_pre` could be improved, somehow?


================
Comment at: llvm/test/MC/ARM/thumb2-ldr.w-str.w.s:81
+str.w r3, [r0, #-4]!
+
+@ CHECK: ldr r3, [r1], #4   @ encoding: [0x51,0xf8,0x04,0x3b]
----------------
DavidSpickett wrote:
> Add pre-increment, no writeback here.
The T3 encodings are already tested in llvm/test/MC/ARM/basic-thumb2-instructions.s.
https://github.com/llvm/llvm-project/blob/f8c1f3b14ad988fd786b896b4b342606bdc760cf/llvm/test/MC/ARM/basic-thumb2-instructions.s#L989-L990


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96632/new/

https://reviews.llvm.org/D96632