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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:2946
+        Inst.addOperand(MCOperand::createImm(0));
+      else if (const MCConstantExpr *CE =
+                   dyn_cast<MCConstantExpr>(Memory.OffsetImm)) {
----------------
else if (const auto *CE = ...)

(Prefer `auto` when using `dyn_cast`; the type info is otherwise mostly redundant).


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3000
+        Inst.addOperand(MCOperand::createImm(0));
+      else if (const MCConstantExpr *CE =
+                   dyn_cast<MCConstantExpr>(Memory.OffsetImm)) {
----------------
else if (const auto *CE =...


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3013
 
-    int64_t Val = Memory.OffsetImm ? Memory.OffsetImm->getValue() : 0;
     Inst.addOperand(MCOperand::createReg(Memory.BaseRegNum));
----------------
DavidSpickett wrote:
> Here we check that Memory.OffsetImm is non zero (non null?) and we don't after. Does that check get done later anyway?
> 
> Ditto for the other functions that look like this one.
other functions have

```
 if (!Memory.OffsetImm)
      Inst.addOperand(MCOperand::createImm(0));
```
if `addExpr` doesn't handle their more specific cases.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3059
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm)) {
----------------
const auto *CE =...


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3089-3090
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm)) {
+      int32_t Val = CE->getValue() / 2;
----------------
const auto *CE = ...


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3140-3141
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+      Inst.addOperand(MCOperand::createImm(CE->getValue() / 4));
----------------
const auto *CE =


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3234-3235
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+      Inst.addOperand(MCOperand::createImm(CE->getValue() / 4));
----------------
const auto *CE


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3246
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm))
----------------
const auto *CE


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3264-3265
+      Inst.addOperand(MCOperand::createImm(0));
+    else if (const MCConstantExpr *CE =
+                 dyn_cast<MCConstantExpr>(Memory.OffsetImm))
+      Inst.addOperand(MCOperand::createImm(CE->getValue() / 4));
----------------
const auto *CE


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5943
     // instructions.
-    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(Offset);
-    if (!CE)
-      return Error (E, "constant expression expected");
-
-    // If the constant was #-0, represent it as
-    // std::numeric_limits<int32_t>::min().
-    int32_t Val = CE->getValue();
-    if (isNegative && Val == 0)
-      CE = MCConstantExpr::create(std::numeric_limits<int32_t>::min(),
-                                  getContext());
+    if (const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(Offset)) {
+      // If the constant was #-0, represent it as
----------------
const auto *CE


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5952-5957
+      Operands.push_back(ARMOperand::CreateMem(
+          BaseRegNum, CE, 0, ARM_AM::no_shift, 0, 0, false, S, E));
+
+    } else
+      Operands.push_back(ARMOperand::CreateMem(
+          BaseRegNum, Offset, 0, ARM_AM::no_shift, 0, 0, false, S, E));
----------------
The tails of these two branches match, other than the second operand. Can you declare a local `const MCExpr *`, simply assign to it from both branches, then sink the call to `push_back` to be executed after both branches merge?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:984-1021
   if (!MO.isReg()) {
     Reg = CTX.getRegisterInfo()->getEncodingValue(ARM::PC);   // Rn is PC.
-    Imm12 = 0;
 
     if (MO.isExpr()) {
       const MCExpr *Expr = MO.getExpr();
       isAdd = false ; // 'U' bit is set as part of the fixup.
 
----------------
seeing:

if !x:
  foo()
else:
  bar()

is a code smell.  We should prefer:

if x:
  bar()
else:
  foo()

===

There seems like a fair amount of duplication between sub branches of these nested conditionals, at least handling an operand that is an expression.  I wonder if we could reduce the duplication somehow?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:988-996
       const MCExpr *Expr = MO.getExpr();
       isAdd = false ; // 'U' bit is set as part of the fixup.
 
       MCFixupKind Kind;
       if (isThumb2(STI))
         Kind = MCFixupKind(ARM::fixup_t2_ldst_pcrel_12);
       else
----------------
can you sink the subexpression `MO.getExpr()` into the argument list for `MCFixup::create` here, so that it matches below?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:1018-1019
+      isAdd = false; // 'U' bit is set as part of the fixup.
+      MCFixupKind Kind = MCFixupKind(ARM::fixup_arm_ldst_imm_12);
+      Fixups.push_back(MCFixup::create(0, MO1.getExpr(), Kind, MI.getLoc()));
+    }
----------------
What if we're in Thumb2 mode? I think we need a different fixup kind, like L991-996.

Do we need to increment MCNumCPRelocations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916