[PATCH] D81948: [LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34

[Sean Fertile via Phabricator via llvm-commits]

sfertile accepted this revision as: sfertile.
sfertile added a comment.

LGTM.



================
Comment at: lld/ELF/Arch/PPC64.cpp:1019
+  case R_PPC64_GOT_PCREL34: {
+    uint64_t si0Mask = 0x00000003FFFF0000;
+    uint64_t si1Mask = 0x000000000000FFFF;
----------------
MaskRay wrote:
> sfertile wrote:
> > @MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto for the suggested test change (ie labels instead of sections)
> In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use `const`. It seems to me that is a good idea. Many other components of LLVM use `const` whenever appropriate.
Big +1 to judiciously adding const wherever appropriate.


================
Comment at: lld/test/ELF/ppc64-got-pcrel34-reloc.s:33
+# RELA:        0009a578  0000000200000014 R_PPC64_GLOB_DAT       0000000000000000 glob_int8 + 0
+.section .GLOB_INT_PCREL_OFFSET,"ax", at progbits
+	pld 3, glob_int8 at got@PCREL(0), 1
----------------
stefanp wrote:
> MaskRay wrote:
> > sfertile wrote:
> > > It this test just to show the linker doesn't modify the load instruction that follows the PC-relative load from the got?
> > If you want to check not TOC (a variant of GOT) optimization is performed, use -no-pie (default) or -pie, instead of -shared.
> Yes, this test is just to make sure that we don't make changes to the `lwa`. It may not be necessary from that perspective because that instruction does not have a relocation on it so there really is no risk of this problem. However, in my head they are the part of the same set so I want to check them both.
> 
> 
> I'm not trying to check for not TOC. We have removed all of the TOC references on the compiler side. Or, maybe I didn't understand your comment...
If it was to test we don't try to perform toc-optimizations then I was going to say you need to make it an executable like MaskRay suggested. But since its to make sure we don't modify past the end of the prefixed instructions its good the way you have it, this test won't change when you do go to implement the got optimizations which is how we want it I think 👍 


================
Comment at: lld/test/ELF/ppc64-reloc-got-pcrel34.s:18
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t | FileCheck %s
+
+.text
----------------
If the 'SYMBOL' checks are to verify the referenced symbols make it into the dynamic symbol table maybe have a first check line:

`SYMBOL: Symbol table '.dynsym' contains 4 entries:`


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

https://reviews.llvm.org/D81948