[PATCH] D82816: [LLD][PowerPC] Implement R_PPC64_REL24_NOTOC calls, callee also has no TOC

[Victor Huang via Phabricator via llvm-commits]

NeHuang marked 5 inline comments as done and an inline comment as not done.
NeHuang added a comment.

Thanks @MaskRay.

- Inserted `fatal()` for unsupported features.
- Cleaned up test to keep necessary instructions and combine cases.
- Fixed some nits in comment.



================
Comment at: lld/ELF/Arch/PPC64.cpp:1043
+  // FIXME: Remove the assertions once the call protocols are supported.
+  assert(!(type == R_PPC64_REL24_NOTOC && (s.stOther >> 5) > 1) &&
+         "Unsupported protocol: RelType is R_PPC64_REL24_NOTOC and the callee "
----------------
MaskRay wrote:
> Note: assert should only be used for logically unreachable code, i.e. if the implementation is not buggy, the negative code path should not trigger.
> 
> You can use `fatal(...)` for unimplemented features. Please use all lowercase messages.
Added `fatal()`. I am using lower cases for the messages except the relocation name. 


================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-pcrel-callee-hidden.s:111
+  mr 30, 5
+  bl callee2_stother1_local at notoc
+  add 3, 3, 30
----------------
MaskRay wrote:
> IIUC, you can merge caller2 and caller2_tailcall and delete all instructions except:
> 
> ```
> .localentry caller2_tailcall, 1
> bl callee2_stother1_local at notoc
> b callee2_stother1_local at notoc
> ```
Good point. Merged them for all the test. 


================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-pcrel.s:42
+
+# CHECK-LABEL: caller1
+# CHECK: 10010018: bl 0x10010000
----------------
MaskRay wrote:
> `caller1` is not a good FileCheck label.
> 
> `<caller1>:` is. It is unique in the llvm-objdump output.
> 
> Please fix all the occurrences.
Added the fix for all the occurrences. 


================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-pcrel.s:52
+callee1_stother0_local:
+  mullw 3, 3, 3
+  extsw 3, 3
----------------
MaskRay wrote:
> I think these instructions are entirely irrelevant to the test and should be deleted to make tests more focused/readable.
> 
> mullw 3, 3, 3
> ext3sw 3,3
> 
> Please check some newer x86-64-* and aarch64-* tests. They don't have such setup instructions.
> 
> But please keep the instruction after `bl ... at notoc` to make it clear that the next instruction is not special as in R_PPC64_REL24
> 
> I think cleaning up the instructions can make the test smaller by at least one half.
Thanks for the advice. Only keep necessary instructions to make the test smaller. 


================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-pcrel.s:153
+.section .text_extern_stother0, "ax", %progbits
+caller3:
+  .localentry caller3, 1
----------------
MaskRay wrote:
> ppc64-pcrel-call-to-pcrel-callee-hidden.s changes the symbol bindings to STB_GLOBAL.
> Do these symbols need `.globl`?
Good catch. They suppose to be `.local` by default. Updated `ppc64-pcrel-call-to-pcrel-callee-hidden.s` to make them consistent. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82816