[PATCH] D94627: [PowerPC][PC Rel] Implement option to omit P10 instructions from stubs

[Amy Kwan via Phabricator via cfe-commits]

amyk added a comment.

A few general comments.



================
Comment at: lld/ELF/Config.h:74
+// For --power10-stub
+enum class P10Stub { Default, No };
+
----------------
We have a "yes", but does it need to be here, too?


================
Comment at: lld/ELF/Driver.cpp:763
 
+static P10Stub getP10StubOpt(opt::InputArgList &args) {
+
----------------
Add documentation for the function.


================
Comment at: lld/ELF/Driver.cpp:776
+  }
+
+  return P10Stub::Default;
----------------
Do we need to handle `power10_stubs`?


================
Comment at: lld/ELF/Options.td:450
+def power10_stubs_eq:
+  J<"power10-stubs=">, HelpText<"Enables Power10 instr in all stubs without options, "
+                     "options override previous flags."
----------------
nit: maybe use the full word `instructions` here and in the following lines.


================
Comment at: lld/ELF/Options.td:452
+                     "options override previous flags."
+                     "auto: Default"
+                     "no:   No Power10 instr in stubs"
----------------
Describe the default behaviour?


================
Comment at: lld/ELF/Thunks.cpp:920
   const int64_t offset = computeOffset();
-  write32(buf + 0, 0xf8410018);                         // std  r2,24(r1)
+  write32(buf + 0, 0xf8410018);                           // std  r2,24(r1)
   // The branch offset needs to fit in 26 bits.
----------------
Unrelated change?


================
Comment at: lld/ELF/Thunks.cpp:925
   } else if (isInt<34>(offset)) {
-    const uint64_t paddi = PADDI_R12_NO_DISP |
-                           (((offset >> 16) & 0x3ffff) << 32) |
-                           (offset & 0xffff);
-    writePrefixedInstruction(buf + 4, paddi); // paddi r12, 0, func at pcrel, 1
-    write32(buf + 12, MTCTR_R12);             // mtctr r12
-    write32(buf + 16, BCTR);                  // bctr
+    int inst2;
+    if (config->Power10Stub == P10Stub::No) {
----------------
Can we maybe try to have a more descriptive name than `inst2`? (here and in the other places)


================
Comment at: lld/test/ELF/ppc64-toc-call-to-pcrel.s:23
+
+## The point of this test is to make sure that when a function with TOC access
 # a local function with st_other=1, a TOC save stub is inserted.
----------------
No need for the extra `#`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94627