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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
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();
----------------
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.


================
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();
----------------
I think this is Thumb2 t2ldr_pcrel_imm12_asmoperand in ARMInstrThumb2.td


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3007
+        // Special case for #-0
+        if (Val == std::numeric_limits<int32_t>::min())
+          Val = 0;
----------------
Looks like a few changes are just formatting (clang-format?) While it doesn't bother me personally, I have seen some people prefer that pure formatting changes are committed separately with a [NFC] tag as it minimises the diff when trying to bisect a problem. Maybe worth committing these bits first in a separate [NFC]


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3058
     Inst.addOperand(MCOperand::createReg(Memory.BaseRegNum));
-    Inst.addOperand(MCOperand::createImm(Val));
+    // The lower two bits are always zero and as such are not encoded.
+    if (!Memory.OffsetImm)
----------------
I think this comment makes more sense when it immediately precedes CE->getValue() / 4; on line 3062


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3137
     Inst.addOperand(MCOperand::createReg(Memory.BaseRegNum));
-    Inst.addOperand(MCOperand::createImm(Val));
+    // The lower two bits are always zero and as such are not encoded.
+    if (!Memory.OffsetImm)
----------------
Same as before, can the comment be moved to immediately before line 3141?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3234
+    else if (const auto *CE = dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+      Inst.addOperand(MCOperand::createImm(CE->getValue() / 4));
+    else
----------------
To be consistent I suppose we ought to add the "// The lower two bits are always zero and as such are not encoded."


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3262
+    else if (const auto *CE = dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+      Inst.addOperand(MCOperand::createImm(CE->getValue() / 4));
+    else
----------------
Another opportunity for // The lower two bits are always zero and as such are not encoded.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:991
+      isAdd = false; // 'U' bit is set as part of the fixup.
+      MCFixupKind Kind = MCFixupKind(ARM::fixup_arm_ldst_abs_12);
+      Fixups.push_back(MCFixup::create(0, MO1.getExpr(), Kind, MI.getLoc()));
----------------
One thing that could be confusing is that there is only an Arm fixup here, whereas in the else if (MO.isExpr()) { ... } there are both Arm and Thumb fixups.

If it is only possible to be here for an Arm Instruction, my apologies I haven't had time to double check the reference manual to see if Thumb has more restricted addressing modes than Arm, can we put a comment here? Possibly an assert that the STI is Arm state?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916