[PATCH] D39910: [ARM] Issue an eror when non-general-purpose registers used in address operands (alternative)

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: lib/Target/ARM/ARMInstrInfo.td:608
+    let Name = "RegShiftedReg";
+    let DiagnosticString = "shifted register operand must use two registers in "
+                           "the range [r0, r15]";
----------------
olista01 wrote:
> I've been trying to standardise the grammar of these messages on "operand must be a <type> ..." rather than "<type> operand must be ...", see D36689.
The reason for this phrasing is that it makes the diagnostic more clear, for example, when assembling
```
add r3, r0, s1, lsl r6
```
the error message is like
```
addr.s:17:17: note: shifted register operand must use two registers in the range [r0, r15]
add r3, r0, s1, lsl r6
                ^
```

with the caret pointing to the shift amount, whereas it's the `s1` register that needs to be fixed.



================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1157
+    if (Memory.BaseRegNum &&
+        !ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Memory.BaseRegNum))
+      return false;
----------------
olista01 wrote:
> There is no code here checking for anything more specific than GPR (other than the TBB/TBH operands), but there are lots of instructions that cannot use sp or pc.
Right. So, I have to draw the line somewhere and currently it's at not allowing non-GPRs in address and shifted register operands, and updating the diagnostics to match the code (in that sense, I probably shouldn't have made the changes for TBB/TBH).

Perhaps we would like to reject syntaxes, which generate an encoding with unpredictable behavior, or perhaps it's not worth the trouble. To me it seems like a subject for further discussion and a follow-up patch(es).


https://reviews.llvm.org/D39910