[PATCH] D114172: [ARM] implement support for ALU/LDR PC-relative group relocations

[Ard Biesheuvel via Phabricator via llvm-commits]

ardb added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:425
 
-// Rotate a 32-bit unsigned value left by a specified amt of bits.
-static uint32_t rotl32(uint32_t val, uint32_t amt) {
-  assert(amt < 32 && "Invalid rotate amount");
-  return (val << amt) | (val >> ((32 - amt) & 31));
+static unsigned getRemainderForGroup(unsigned group, uint32_t val,
+                                     uint32_t &rem) {
----------------
peter.smith wrote:
> Was about to comment that this was more like getLeadingZerosForGroup until seeing the reference parameter. One possibility is to return std::pair<rem, lz> and then use something like:
> ```
> std::tie<imm, lz> = getRemAndLZForGroup(group, val);
> ```
> In the main body.
> Was about to comment that this was more like getLeadingZerosForGroup until seeing the reference parameter. One possibility is to return std::pair<rem, lz> and then use something like:
> ```
> std::tie<imm, lz> = getRemAndLZForGroup(group, val);
> ```
> In the main body.

Yeah, I wasn't too happy with this myself, but given that the LDR_PC_G1/2 relocations are always used in combination with ALU_PC_G0(1) to encode a single PC-relative symbol reference, I think it is important for readability that both code paths use the same helper to mask off the leading bits. And since the LDR version doesn't care about the shift, it only receives a single return value.

So if your suggested change is considered to be the best way to achieve this, I'm happy to adopt it.


================
Comment at: lld/ELF/Arch/ARM.cpp:451
+    imm = rotr32(imm, 24 - lz);
+    rot = ((lz + 8) & 30) << 7;
+  }
----------------
peter.smith wrote:
> IIUC I think the & 30 isn't strictly necessary as lz is always even when returned from getRemainderForGroup and lz + 8 can't be more than 30. No objections to keeping it in though.
> IIUC I think the & 30 isn't strictly necessary as lz is always even when returned from getRemainderForGroup and lz + 8 can't be more than 30. No objections to keeping it in though.

I realized the same after posting the second revision. I'll drop it in the next one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114172