[PATCH] D108280: [JITLink] Optimize GOTPCRELX Relocations

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

This is very cool -- thanks Stephen!

Please address the inline comments, otherwise LGTM!



================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:82-84
+        auto *BlockData = reinterpret_cast<uint8_t *>(
+                              const_cast<char *>(B->getContent().data())) +
+                          E.getOffset();
----------------
`BlockData` should be `FixupData`.

I know that was my name originally, but now that this function is getting more attention we should fix my old mistakes. ;)


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:95
         JITTargetAddress TargetAddr = GOTTarget.getAddress();
-
+        JITTargetAddress EdgeAddr = B->getAddress() + E.getOffset();
         int64_t Displacement = TargetAddr - EdgeAddr + 4;
----------------
The preferred way to express this is `JITTargetAddress EdgeAddr = B->getFixupAddress(E)`.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:100-101
+
+        if (!(TargetInRangeForImmU32 || DisplacementInRangeForImmS32))
+          continue;
+
----------------
I assume the rationale here is "if neither of these is true then there's no point looking at the instruction sequence more closely"? I would add a comment for that to make it clear.


================
Comment at: llvm/test/ExecutionEngine/JITLink/X86/ELF_x86-64_small_pic_relocations.s:71-81
 # Test GOTPCREL handling. We want to check both the offset to the GOT entry and its
 # contents.
 # jitlink-check: decode_operand(test_gotpcrel, 4) = \
 # jitlink-check:     got_addr(elf_sm_pic_reloc.o, named_data) - next_pc(test_gotpcrel)
 # jitlink-check: *{8}(got_addr(elf_sm_pic_reloc.o, named_data)) = named_data
 
         .globl test_gotpcrel
----------------
Could you add to the comment to note that `leal` is used here to suppress optimization (in case anybody looks at it and wonders why it isn't a mov).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108280