[PATCH] D117614: [lld][ELF] Add support for ADRP+ADD optimization for AArch64

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the update. The code looks great. I just have two comments whether the tests can be strengthened. I will not hold off the patch if that is too difficult.



================
Comment at: lld/ELF/Arch/AArch64.cpp:622
+  uint32_t adrpDestReg = adrpInstr & 0x1f;
+  uint32_t addDestReg = addInstr & 0x1f;
+  uint32_t addSrcReg = (addInstr >> 5) & 0x1f;
----------------
If there are multiple tests covering the bitmask, it would be nice to mutate them a bit to cover every bit of 0x1f.


================
Comment at: lld/ELF/Arch/AArch64.cpp:630
+  int64_t val = sym.getVA() - (secAddr + addRel.offset);
+  if (val < -1024 * 1024 || val >= 1024 * 1024)
+    return false;
----------------
It might be difficult, but are you able to come up cases exercising the edge condition of if (val < -1024 * 1024 || val >= 1024 * 1024) ?
ppc64-long-branch.s uses this idea. If has branch distances just within the range and just outside of the range.

I raised this because in the past I have fixed some bugs related to the edge case. It'd be nice to have tests covering the cases in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117614