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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:866
+    // LDRD/LDRH/LDRSB/LDRSH (literal) u = bit23 unsigned imm8
+    bool u = read32le(buf) & 0x00800000;
+    uint32_t imm4l = read32le(buf) & 0xf;
----------------
Call `read32le(buf)` once instead of three times.


================
Comment at: lld/ELF/Arch/ARM.cpp:431
+    rem = val;
+    val &= (uint64_t)0xffffffU >> lz;
+  } while (group--);
----------------
ardb wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > 
> > > val is uint32_t. Remove `(uint64_t)`?
> > Is `(uint64_t)0xffffff >> 32` possible? If lz can be 32, casting to `(uint64_t)` LGTM but it needs a comment. I am not familiar with the encoding and do not know whether lz can be 32.
> Yes, lz will be 32 if val == 0. This may happen when using multiple groups if the value can be fully encoded by the previous group(s).
> 
> Note that in that case, doing &= should not have any effect, but a 32-bit shift of a 32-bit type is better avoided entirely, I'd assume?
Unaddressed: needs a uint64_t cast to fix UB. `val &= (uint64_t)0xffffff >> lz;  // lz may be 32` 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114172