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

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

I'm not familiar with how fixups work so someone else should check that part. What you do with them looks fine but I don't know if those names are llvm internal or we have to match some published spec elsewhere.



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


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5954
+          BaseRegNum, CE, 0, ARM_AM::no_shift, 0, 0, false, S, E));
+
+    } else
----------------
Nit: remove the newline


================
Comment at: llvm/test/MC/ARM/arm-memory-instructions-immediate.s:4
+
+  .syntax unified
+
----------------
Unindent this or indent the .space line below.

Also could you add a comment describing the purpose? To make it clear this is looking at a specific kind of immediates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916