[PATCH] D96632: [THUMB2] add .w suffixes for ldr/str (immediate) T4

[David Spickett via Phabricator via llvm-commits]

DavidSpickett 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!",
----------------
nickdesaulniers wrote:
> 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.
Makes sense now.


================
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) {
----------------
nickdesaulniers wrote:
> 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?
I'm sure it could be improved but out of scope here. My concern was that we would leave a gap where valid input would be encoded to T4 but not T3. However, clang already accepts the T3 input so it's not an issue.

```
ldr.w r3, [r1, #4095]
$ ./bin/clang --target=arm-none-eabi -march=armv7-a /tmp/test.s -c -o /tmp/test.o -Wa,-mthumb
```


================
Comment at: llvm/test/MC/ARM/thumb2-ldr.w-str.w.s:123
+ldr.w r0, [r1], #4
+ldr.w sp, [r1], #4 @ TODO: GAS warns for this
+ldr.w pc, [r1], #4
----------------
Do you have a plan for how to implement these? I assume it's complicated in the same way the alias constraints are.


================
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]
----------------
nickdesaulniers wrote:
> 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
 


================
Comment at: llvm/test/MC/ARM/thumb2-ldr.w-str.w.s:91
+
+@@ LDR pre-increment
+@ Vary Rt.
----------------
DavidSpickett wrote:
> You should check pre-increment without writeback, that would use the T3 encoding and have a greater immediate range.
 


================
Comment at: llvm/test/MC/ARM/thumb2-ldr.w-str.w.s:296
+
+@@ STR pre-increment
+@ Vary Rt.
----------------
DavidSpickett wrote:
> pre-increment without writeback
Addressed above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96632