[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/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.
 
----------------
nickdesaulniers wrote:
> 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?
(I'm tempted to go and pre-commit the previous change as that code smell is pervasive throughout this file).

Looks better, now I see:

```
if MO.isReg():
  foo()
else:
  if MO.isExpr():
    bar()
  else:
    baz()
```
should be
```
if MO.isReg():
  foo()
else if MO.isExpr():
  bar()
else: # MO.isImm
  baz()
```
Then it's obvious that the MCOperand is an immediate, and removes one level of unnecessary indentation.


================
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()));
+    }
----------------
jcai19 wrote:
> 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?
> 
> 
> I will look into the thumb mode but do we care about it at this moment?

We don't have a pressing need for THUMB2 support, but I worry that someone using clang may find the wrong fixup(relocation?) applied in the event they are assembling a THUMB2 source with a symbolic operand, due to this change. Perhaps a check for `isThumb2(STI)` then `assert` if thumb, or skip generating the fixup? (Or maybe another reviewer has thoughts?)

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

Looks like it's just a stat (so probably doesn't really matter too much):

```
STATISTIC(MCNumCPRelocations, "Number of constant pool relocations created.");
```

Since it's an immediate, it's encoded in the instruction; not a constant pool.  But seeing how it's incremented elsewhere throughout this TU, it looks like everytime we have an Expr MCOperand, we bump it. So I think you should here, too. Maybe other reviewers can help clarify?



Seeing how similar the users of `MCNumCPRelocations` makes me think there's a whole lot of other instruction formats with this issue that you're fixing.  Though I think it's fine that we start with ldr/str.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:996-1007
+      Reg = CTX.getRegisterInfo()->getEncodingValue(ARM::PC); // Rn is PC.
       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
         Kind = MCFixupKind(ARM::fixup_arm_ldst_pcrel_12);
----------------
I wonder why we aren't consistent about the use of `getEncodingValue` when assigning to `Reg`? Are these actually different values? (I recognize your change doesn't introduce this, just curious).


================
Comment at: llvm/test/MC/ARM/arm-memory-instructions-immediate.s:25
+// ERR:[[#@LINE-1]]:5: error: unsupported relocation on symbol
+.endif
----------------
if you add `.thumb` you can repeat some tests for THUMB (I would then add `.arm` before the initial group of instructions to clearly delineate between ARM mode and THUMB. `getAddrModeImm12OpValue` has pre-existing checks for THUMB, so I suspect we should be able to add coverage for your additions with THUMB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916