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

Mon Mar 22 12:09:27 PDT 2021

jcai19 added inline comments.

================
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);
----------------
nickdesaulniers wrote:
> 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).
Yeah I had the same question, so I changed the original code a little bit to make it more obvious for others to notice. I wonder if this is a typo?

================
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:
> 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.
> 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?)

I did some more digging and the thumb mode of these instructions are actually handled by different functions, so right now comping them in thumb mode (either with thumbv7 in the target triple or .thumb) will crash the compiler with assertion failures. I have been looking into supporting thumb mode a little bit, and found extra complexity: while the instructions supported by this patch (ldr/ldrb, str/strb) have one encoding in arm mode, they have multiple encodings in thumb mode, depending on the value of the immediate operand,  or if it is (un)signed. Some of these encodings are 16 bits while the others 32. IAS associate each encoding with a different opcode and tries to decide the opcode before the fixups are resolved, so it is difficult to decide the precise encoding when expressions are present. One solution I could think of right now is to always match such instructions to the shortest encoding so they don't unnecessarily generate 32-bit instructions when 16-bit version can be used, at the cost of not being able to support expressions with a result outside of the range. WDYT? Also I think it may be easier to implement the thumb mode support in a different CL so we don't have to deal with two complications at the same time.

PS: I re-examined my CL for supporting expressions in b.w instructions (https://reviews.llvm.org/D97568). b.w has two encoding in thumb mode but both are 32-bit instructions and they do not look that differently, so IAS somehow has only one opcode for both of them (verified with --debug-only=asm-matcher). So b.w probably don't have this issue.

> 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?

+1.

> 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.

Yes there are other memory operations with similar issues but hopefully once we have this checked in it would be much easier to fix those.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98916