[PATCH] D98916: [ARM] support symbolic expression as immediate in memory instructions

[Jian Cai via Phabricator via llvm-commits]

jcai19 added a comment.

In D98916#2673445 <https://reviews.llvm.org/D98916#2673445>, @peter.smith wrote:

> I have a one major concern, and a number of minor nits that could easily be addressed prior to commit if necessary.
>
> The major concern is that if the intent is to just permit expressions for Arm state LDR instructions (addition of one fixup) then a lot of code in the AsmParser looks like it is permitting expressions for Thumb and Thumb2 LDR instructions that wouldn't use the one new fixup. Can you double check the predicates to make sure that they apply to the instructions you want to permit expressions on? It is possible I've got something wrong though. If I'm right then where we would have had errors had someone use an expression then we may get an internal error or crash as the backend won't be able to handle the expression. My suggestion would be to take out from ARMAsmParser all the code permitting exceptions except for the ARM instructions that support the fixup. Adding support for expressions to all the ARM and Thumb instructions is possible but that would probably be better done over several patches. If I'm wrong please ignore me and sorry for the noise.

So the current patch only accepts symbolic expressions in isMemImm12Offset, so only instructions that call this function would be able to accept symbolic expressions, namely, `ldr`, `ldrb`, `str` and `strb` in ARM mode. All the other is* function would return false when symbolic expressions are spotted, including the thumb mode of the support instructions.

  $ cat foo.s 
  .syntax unified
  .thumb
  0:
  .space 2048
  1:
  .space 20
  2:
  str r0, [r1, #(2b-1b)]
  
  llvm-mc -triple=armv7a -filetype=obj foo.s -o ias.o
  foo.s:8:1: error: invalid instruction, any one of the following would fix this:
  str r0, [r1, #(2b-1b)]
  ^
  foo.s:8:9: note: invalid operand for instruction
  str r0, [r1, #(2b-1b)]
          ^
  foo.s:8:1: note: instruction requires: arm-mode
  str r0, [r1, #(2b-1b)]



================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1110
       if(Memory.BaseRegNum != ARM::PC) return false;
-      Val = Memory.OffsetImm->getValue();
+      if (const auto *CE = dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+        Val = CE->getValue();
----------------
peter.smith wrote:
> This predicate is only called for the ThumbMemPC class (see ARMInstrThumb.td) if the intent is to allow expressions only in certain Arm load and store instructions then I think this may be letting through expressions where the backend won't support them. It is highly possible that there are no tests that fail because there may be no expect fail tests written.
This change here should not alter the semantics, i.e. only MCConstantExprs get through. Other types of MCExprs would trigger the following `return false` case.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1502
     if (!Memory.OffsetImm) return true;
-    int64_t Val = Memory.OffsetImm->getValue();
-    return (Val > -4096 && Val < 4096) ||
-           (Val == std::numeric_limits<int32_t>::min());
+    if (const auto *CE = dyn_cast<MCConstantExpr>(Memory.OffsetImm)) {
+      int64_t Val = CE->getValue();
----------------
peter.smith wrote:
> I think this is Thumb2 t2ldr_pcrel_imm12_asmoperand in ARMInstrThumb2.td
Same as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916