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

[Stefan Pintilie via Phabricator via cfe-commits]

stefanp added a comment.

There is one more thing that does not look quite right. It may be an encoding problem for one of the instructions. I found it in the test but you will have to trace it back to the place where it is generated in order to fix it.



================
Comment at: lld/ELF/Driver.cpp:770
+
+  StringRef SelectedOpt = args.getLastArgValue(OPT_power10_stubs_eq);
+
----------------
MaskRay wrote:
> `value`
You may even be able to inline this since it is only used in one place that I can see.


================
Comment at: lld/ELF/Options.td:450
+def power10_stubs_eq:
+  J<"power10-stubs=">, HelpText<"Enables Power10 instsructions in all stubs without options, "
+                     "options override previous flags."
----------------
nit:
Is this line too long? Or does it just appear too long in my browser. If the length is ok ignore my comment. 


================
Comment at: lld/ELF/Thunks.cpp:930
+        uint64_t addi = ADDI_R12_TO_R12_NO_DISP | (tocOffset & 0xffff);
+        const uint64_t addis = ADDIS_R12_TO_R2_NO_DISP | ((tocOffset >> 16) & 0xffff);
+        write32(buf + 4, addis); // addis r12, r2 , top of offset
----------------
nit:
You can mark both as const.
Also below in this same function.


================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-toc.s:55
+# CHECK-NOP10-NEXT:  mtlr 12
+# CHECK-NOP10-NEXT:  addis 12, 12, -1
+# CHECK-NOP10-NEXT:  addi 12, 12, -24
----------------
This is not correct.
You should be adding to `r11` and not `r12` here.
ie.
```
addis 12, 11, -1
```
The encoding above may not be correct.


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

https://reviews.llvm.org/D94627