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

[Jian Cai via Phabricator via llvm-commits]

jcai19 added a comment.

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

> The fixup code looks reasonable to me as it is mostly reusing the same logic as the pc-relative fixup. The names are internal to LLVM so there isn't any particular standard. I think this fixup would correspond to the relocation R_ARM_ABS12 https://github.com/ARM-software/abi-aa/blob/master/aaelf32/aaelf32.rst#relocation-codes so it may be worth using fixup_arm_ldst_abs_12 but that is only a suggestion. I wouldn't expect the assembler to use the relocation as the range is so short that any attempt to use it would likely result in an out of range error.

Thanks for the suggestion, I've updated the name.



================
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));
----------------
nickdesaulniers wrote:
> 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.
Yes [addExpr](https://llvm.org/doxygen/ARMAsmParser_8cpp_source.html#l02388) would take care of the non-null case. For the cases I could not call addExpr directly, I've handled the case separately.


================
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()));
+    }
----------------
nickdesaulniers wrote:
> What if we're in Thumb2 mode? I think we need a different fixup kind, like L991-996.
> 
> Do we need to increment MCNumCPRelocations?
> What if we're in Thumb2 mode? I think we need a different fixup kind, like L991-996.

This patch is only intended for some memory instructions in arm mode. I will look into the thumb mode but do we care about it at this moment?

> Do we need to increment MCNumCPRelocations?

The expressions of interest here are really those would produce immediate values, so I'm not sure if I should count them as relocation. WDYT?




================
Comment at: llvm/test/MC/ARM/arm-memory-instructions-immediate.s:11
+foo:
+    ldr r0, [r1, #(1b - 0b)]
+// CHECK-NEXT: ldr r0, [r1, #1024]
----------------
peter.smith wrote:
> Could we add some test cases for negative and zero offsets. It looks like this is handled in the fixup code but it would be good to have some tests to make sure it isn't broken in the future.
Ah yes. I did test negative numbers locally but forgot to add it here, thanks for the reminder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916