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

2021-02-01 Thread 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 & 0x);
+const uint64_t addis = ADDIS_R12_TO_R2_NO_DISP | ((tocOffset >> 16) & 
0x);
+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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-01-27 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

> Implement option to omit P10  instructions from 
> stubs

Please mention the option name directly.
Please avoid P10  - the abbreviation is confusing.




Comment at: lld/ELF/Config.h:74
+// For --power10-stub
+enum class P10Stub { Default, No };
+

In this case it can be a simple boolean



Comment at: lld/ELF/Driver.cpp:767
+
+  // This handles the --no-power10-stubs option.
+  

Delete the comment. it is not useful



Comment at: lld/ELF/Driver.cpp:770
+
+  StringRef SelectedOpt = args.getLastArgValue(OPT_power10_stubs_eq);
+

`value`



Comment at: lld/ELF/Thunks.cpp:1005
+nextInstOffset = 8;
+  } else { //CONVERT
+uint32_t off = destination.getVA(addend) - getThunkTargetSym()->getVA() - 
8;

Delete the stray comment or clarify.



Comment at: lld/ELF/Thunks.cpp:1052
+  int nextInstOffset;
+  if (config->Power10Stub == P10Stub::No) { //CONVERT
+uint32_t off = destination.getVA(addend) - getThunkTargetSym()->getVA() - 
8;

Delete the stray comment or clarify.



Comment at: lld/ELF/Thunks.cpp:1058
+write32(buf + 12, 0x7d8803a6); // mtlr r12
+write32(buf + 16, 0x3d8b | computeHiBits(off));// addis r12,r11,off@ha
+write32(buf + 20, 0x398c | (off & 0x));// addi r12,r12,off@l

space before `//`


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

https://reviews.llvm.org/D94627

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-01-27 Thread Albion Fung via Phabricator via cfe-commits 
Conanap updated this revision to Diff 319715.
Conanap marked 12 inline comments as done.
Conanap added a comment.

Removed lambda functions and made a static function, included a R2SaveStub test 
case that covers if it fits in 34 bits with option --no-power10-stubs.


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

https://reviews.llvm.org/D94627

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Options.td
  lld/ELF/Thunks.cpp
  lld/ELF/Thunks.h
  lld/test/ELF/ppc64-call-reach.s
  lld/test/ELF/ppc64-long-branch-localentry-offset.s
  lld/test/ELF/ppc64-long-branch-pi.s
  lld/test/ELF/ppc64-long-branch-rel14.s
  lld/test/ELF/ppc64-long-branch.s
  lld/test/ELF/ppc64-pcrel-call-to-extern.s
  lld/test/ELF/ppc64-pcrel-call-to-toc.s
  lld/test/ELF/ppc64-plt-stub-compatible.s
  lld/test/ELF/ppc64-tls-pcrel-gd.s
  lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
  lld/test/ELF/ppc64-toc-call-to-pcrel.s
  llvm/include/llvm/Object/ELF.h

Index: llvm/include/llvm/Object/ELF.h
===
--- llvm/include/llvm/Object/ELF.h
+++ llvm/include/llvm/Object/ELF.h
@@ -87,6 +87,9 @@
 
 enum PPCInstrMasks : uint64_t {
   PADDI_R12_NO_DISP = 0x06103980,
+  ADDIS_R12_TO_R2_NO_DISP = 0x3D82,
+  ADDI_R12_TO_R2_NO_DISP = 0x3982,
+  ADDI_R12_TO_R12_NO_DISP = 0x398C,
   PLD_R12_NO_DISP = 0x0410E580,
   MTCTR_R12 = 0x7D8903A6,
   BCTR = 0x4E800420,
Index: lld/test/ELF/ppc64-toc-call-to-pcrel.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel.s
@@ -14,6 +14,12 @@
 # RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
 # RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t | FileCheck %s
 
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
+# RUN: ld.lld -T %t.script %t.o -o %t --no-power10-stubs
+# RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t \
+# RUN: | FileCheck %s
+
 # 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.
 
Index: lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
@@ -11,15 +11,20 @@
 # RUN: llvm-objdump --mcpu=pwr10 --no-show-raw-insn -d %t_be | FileCheck %s
 # RUN: llvm-readelf -s %t_be | FileCheck %s --check-prefix=SYM
 
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %t/asm -o %t.o
+# RUN: ld.lld -T %t/lts %t.o -o %t_le --no-power10-stubs
+# RUN: llvm-objdump --mcpu=pwr10 --no-show-raw-insn -d %t_le | FileCheck %s --check-prefix=NoP10
+# RUN: llvm-readelf -s %t_le | FileCheck %s --check-prefix=SYM
+
 # SYM:  Symbol table '.symtab' contains 9 entries:
 # SYM:  1: 1001 0 NOTYPE  LOCAL  DEFAULT []   1 callee
 # SYM-NEXT: 2: 20020008 0 NOTYPE  LOCAL  DEFAULT []   3 caller
 # SYM-NEXT: 3: 10020008 0 NOTYPE  LOCAL  DEFAULT  2 caller_close
 # SYM-NEXT: 4: 000520020008 0 NOTYPE  LOCAL  DEFAULT  4 caller_far
-# SYM-NEXT: 5: 000520028038 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
+# SYM-NEXT: 5: 000520028040 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
 # SYM-NEXT: 6: 10020020 8 FUNCLOCAL  DEFAULT  2 __toc_save_callee
-# SYM-NEXT: 7: 2002002020 FUNCLOCAL  DEFAULT  3 __toc_save_callee
-# SYM-NEXT: 8: 00052002002020 FUNCLOCAL  DEFAULT  4 __toc_save_callee
+# SYM-NEXT: 7: 2002002032 FUNCLOCAL  DEFAULT  3 __toc_save_callee
+# SYM-NEXT: 8: 00052002002032 FUNCLOCAL  DEFAULT  4 __toc_save_callee
 
 #--- lts
 PHDRS {
@@ -72,6 +77,17 @@
 # CHECK-NEXT:paddi 12, 0, -268501028, 1
 # CHECK-NEXT:mtctr 12
 # CHECK-NEXT:bctr
+
+# NoP10-LABEL: :
+# NoP10: bl 0x20020020
+# NoP10-NEXT:ld 2, 24(1)
+# NoP10-NEXT:blr
+# NoP10-LABEL: <__toc_save_callee>:
+# NoP10-NEXT: std 2, 24(1)
+# NoP10-NEXT:addis 12, 2, -4098
+# NoP10-NEXT:addi 12, 12, 32704
+# NoP10-NEXT:mtctr 12
+# NoP10-NEXT:bctr
 .section .text_caller, "ax", %progbits
 .Lfunc_toc2:
   .quad .TOC.-.Lfunc_gep2
Index: lld/test/ELF/ppc64-tls-pcrel-gd.s
===
--- lld/test/ELF/ppc64-tls-pcrel-gd.s
+++ lld/test/ELF/ppc64-tls-pcrel-gd.s
@@ -42,10 +42,10 @@
 #--- asm
 
 # GD-RELOC: Relocation section '.rela.dyn' at offset 0x100b8 contains 4 entries:
-# GD-RELOC: 01001160  00020044 R_PPC64_DTPMOD64    x + 0
-# GD-RELOC: 01001168  0002004e R_PPC64_DTPREL64    x + 0
-# GD-RELOC: 01001

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

2021-01-27 Thread Stefan Pintilie via Phabricator via cfe-commits 
stefanp added a comment.

I have a few comments. Most of them are nits but there is a functional issue as 
well.

For the testing:
Do we have a test for the `PPC64R2SaveStub` in the situation where the offset 
fits in 34 bits?




Comment at: lld/ELF/Driver.cpp:768
+  // This handles the --no-power10-stubs option.
+  bool NoP10 = args.hasArg(OPT_no_power10_stubs);
+

nit:
You can probably get rid of this temp and have the condition down below change 
to:
```
if (!args.hasArg(OPT_power10_stubs_eq) &&
args.hasArg(OPT_no_power10_stubs))
  return P10Stub::No;
```
Mainly because you only use `NoP10` in one place at this point.



Comment at: lld/ELF/Options.td:445
 
+def power10_stubs: F<"power10-stubs">, HelpText<"Alias for 
--power10-stubs=yes">;
+

nit:
This is not checked in `getP10StubOpt`.
This is no longer an alias for `power10-stubs=yes` it is an alias for default.



Comment at: lld/ELF/Thunks.cpp:932
+write32(buf + 4, addis); // addis r12, 0, top of offset
+write32(buf + 8, addi);  // addi  r12, r12, bottom of offset
+nextInstOffset = 12;

This sequence does not match with what you are using as an offset.
The `tocOffset` is the difference between the TOC pointer (in r2) and the 
address of the destination function (the callee). However, the add instructions 
are using that offset and adding it to zero (`addis r12, 0, top of offset`) 
which is not the TOC pointer. At the end of this sequence the register `r12` 
will not have the address of the callee but simply the difference between that 
address and the TOC pointer.

Since you have a valid `r2` and are computing the offset from the TOC pointer 
you should be adding to that.



Comment at: lld/ELF/Thunks.cpp:935
+  } else {
+write32(buf + 4, addi); // addi r12, 0, offset
+nextInstOffset = 8;

Same here as above.



Comment at: lld/ELF/Thunks.cpp:971
+uint32_t d = destination.getVA(addend);
+uint32_t off = d - getThunkTargetSym()->getVA();
+write32(buf + 0, 0x7c0802a6);  // mflr r12

nit:
The variable `d` is only used in one place so you can just inline it.
Also, for the future try to avoid single letter variable names.



Comment at: lld/ELF/Thunks.cpp:977
+write32(buf + 16, 0x3d8c | ha(off));   // addis r12,r11,off@ha
+write32(buf + 20, 0x398c | (uint16_t)(off)-8); // addi r12,r12,off@l
+nextInstOffset = 24;

You need to have the `off-8` in both cases. The way that this is done is quite 
confusing beause in one case you subtract 8 in the lambda and in the other you 
just subtract here inline. I would say just subtract in the computation of 
`off` and then you don't have to do it twice in two different places.

Secondly, instead of casting to `uint16_t` you are better off just using a mask 
(`off & 0x`).



Comment at: lld/ELF/Thunks.cpp:1010
+uint32_t d = destination.getVA(addend);
+uint32_t off = d - getThunkTargetSym()->getVA();
+write32(buf + 0, 0x7c0802a6);// mflr r12

nit:
Similar to above it is probably better to compute `off` with the `-8` included 
than to add the adjustment to the lambdas. 



Comment at: lld/ELF/Thunks.cpp:1057
+  if (config->Power10Stub == P10Stub::No) {
+auto ha = [](uint32_t v) -> uint16_t { return (v + 0x8000 - 8) >> 16; };
+uint32_t d = destination.getVA(addend);

I see that these lambdas are used in a lot of places to do the same thing. It 
might be better to have a static function instead of defining the same lambda 
three times.



Comment at: lld/ELF/Thunks.cpp:1059
+uint32_t d = destination.getVA(addend);
+uint32_t off = d - getThunkTargetSym()->getVA();
+write32(buf + 0, 0x7c0802a6);  // mflr r12

nit: Same as above. Just add compute the offset with the 8 difference here. 



Comment at: lld/test/ELF/ppc64-pcrel-call-to-toc.s:18
 
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
+# RUN: ld.lld -T %t.script %t.o -o %t --no-power10-stubs

nit:
Do the LE version of the test instead of the BE version. 



Comment at: llvm/include/llvm/Object/ELF.h:94
+  LD_R12_NO_DISP = 0xE980,
+  LD_R12_TO_R12_NO_DISP = 0xE98C,
   MTCTR_R12 = 0x7D8903A6,

Are these two instruction masks used?


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

https://reviews.llvm.org/D94627

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-01-25 Thread Albion Fung via Phabricator via cfe-commits 
Conanap added a comment.

Addressed comments




Comment at: lld/ELF/Config.h:74
+// For --power10-stub
+enum class P10Stub { Default, No };
+

amyk wrote:
> We have a "yes", but does it need to be here, too?
After a bit of discussion, since we don't have a concrete implementation for 
the "yes" option yet, the consensus is to keep it out for now. I'll remove it 
from the help message as well.



Comment at: lld/ELF/Driver.cpp:776
+  }
+
+  return P10Stub::Default;

amyk wrote:
> Do we need to handle `power10_stubs`?
That option is handled by the variable `NoP10`; I'll add a comment to make that 
more clear.



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.

amyk wrote:
> Unrelated change?
This change was made so that the commented instruction would better line up 
with the new instructions added for better readablity. 


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

https://reviews.llvm.org/D94627

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-01-25 Thread Albion Fung via Phabricator via cfe-commits 
Conanap updated this revision to Diff 319140.
Conanap marked 7 inline comments as done.
Conanap added a comment.

Fixed some formatting and comments.


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

https://reviews.llvm.org/D94627

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Options.td
  lld/ELF/Thunks.cpp
  lld/test/ELF/ppc64-call-reach.s
  lld/test/ELF/ppc64-long-branch-localentry-offset.s
  lld/test/ELF/ppc64-long-branch-pi.s
  lld/test/ELF/ppc64-long-branch-rel14.s
  lld/test/ELF/ppc64-long-branch.s
  lld/test/ELF/ppc64-pcrel-call-to-extern.s
  lld/test/ELF/ppc64-pcrel-call-to-toc.s
  lld/test/ELF/ppc64-plt-stub-compatible.s
  lld/test/ELF/ppc64-tls-pcrel-gd.s
  lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
  lld/test/ELF/ppc64-toc-call-to-pcrel.s
  llvm/include/llvm/Object/ELF.h

Index: llvm/include/llvm/Object/ELF.h
===
--- llvm/include/llvm/Object/ELF.h
+++ llvm/include/llvm/Object/ELF.h
@@ -87,7 +87,11 @@
 
 enum PPCInstrMasks : uint64_t {
   PADDI_R12_NO_DISP = 0x06103980,
+  ADDIS_R12_NO_DISP = 0x3D80,
+  ADDI_R12_TO_R12_NO_DISP = 0x398C,
   PLD_R12_NO_DISP = 0x0410E580,
+  LD_R12_NO_DISP = 0xE980,
+  LD_R12_TO_R12_NO_DISP = 0xE98C,
   MTCTR_R12 = 0x7D8903A6,
   BCTR = 0x4E800420,
 };
Index: lld/test/ELF/ppc64-toc-call-to-pcrel.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel.s
@@ -14,6 +14,12 @@
 # RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
 # RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t | FileCheck %s
 
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
+# RUN: ld.lld -T %t.script %t.o -o %t --no-power10-stubs
+# RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t \
+# RUN: | FileCheck %s
+
 # 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.
 
Index: lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
@@ -16,10 +16,10 @@
 # SYM-NEXT: 2: 20020008 0 NOTYPE  LOCAL  DEFAULT []   3 caller
 # SYM-NEXT: 3: 10020008 0 NOTYPE  LOCAL  DEFAULT  2 caller_close
 # SYM-NEXT: 4: 000520020008 0 NOTYPE  LOCAL  DEFAULT  4 caller_far
-# SYM-NEXT: 5: 000520028038 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
+# SYM-NEXT: 5: 000520028040 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
 # SYM-NEXT: 6: 10020020 8 FUNCLOCAL  DEFAULT  2 __toc_save_callee
-# SYM-NEXT: 7: 2002002020 FUNCLOCAL  DEFAULT  3 __toc_save_callee
-# SYM-NEXT: 8: 00052002002020 FUNCLOCAL  DEFAULT  4 __toc_save_callee
+# SYM-NEXT: 7: 2002002032 FUNCLOCAL  DEFAULT  3 __toc_save_callee
+# SYM-NEXT: 8: 00052002002032 FUNCLOCAL  DEFAULT  4 __toc_save_callee
 
 #--- lts
 PHDRS {
Index: lld/test/ELF/ppc64-tls-pcrel-gd.s
===
--- lld/test/ELF/ppc64-tls-pcrel-gd.s
+++ lld/test/ELF/ppc64-tls-pcrel-gd.s
@@ -42,10 +42,10 @@
 #--- asm
 
 # GD-RELOC: Relocation section '.rela.dyn' at offset 0x100b8 contains 4 entries:
-# GD-RELOC: 01001160  00020044 R_PPC64_DTPMOD64    x + 0
-# GD-RELOC: 01001168  0002004e R_PPC64_DTPREL64    x + 0
-# GD-RELOC: 01001170  00030044 R_PPC64_DTPMOD64    y + 0
-# GD-RELOC: 01001178  0003004e R_PPC64_DTPREL64    y + 0
+# GD-RELOC: 01001170  00020044 R_PPC64_DTPMOD64    x + 0
+# GD-RELOC: 01001178  0002004e R_PPC64_DTPREL64    x + 0
+# GD-RELOC: 01001180  00030044 R_PPC64_DTPMOD64    y + 0
+# GD-RELOC: 01001188  0003004e R_PPC64_DTPREL64    y + 0
 
 # GD-SYM:   Symbol table '.dynsym' contains 4 entries:
 # GD-SYM:   2:  0 TLS GLOBAL DEFAULT   UND x
@@ -68,9 +68,9 @@
 # GDTOLE-SYM: 4: 0004 0 TLS GLOBAL DEFAULT 3 y
 
 # GD-LABEL: :
-# GD-NEXT:paddi 3, 0, 352, 1
+# GD-NEXT:paddi 3, 0, 368, 1
 # GD-NEXT:bl
-# GD-NEXT:paddi 3, 0, 356, 1
+# GD-NEXT:paddi 3, 0, 372, 1
 # GD-NEXT:bl
 # GD-NEXT:blr
 # GDTOIE-LABEL: :
Index: lld/test/ELF/ppc64-plt-stub-compatible.s
===
--- lld/test/ELF/ppc64-plt-stub-compatible.s
+++ lld/test/ELF/ppc

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

2021-01-22 Thread 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) & 0x3) << 32) |
-   (offset & 0x);
-writePrefixedInstruction(buf + 4, paddi); // paddi r12, 0, func@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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-01-13 Thread Albion Fung via Phabricator via cfe-commits 
Conanap created this revision.
Conanap added reviewers: stefanp, nemanjai, PowerPC.
Conanap added projects: LLVM, clang, PowerPC.
Herald added subscribers: dang, kbarton, arichardson, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: MaskRay.
Conanap requested review of this revision.

Implemented the option to omit Power10 instructions from save stubs via the 
option `--no-power10-stubs` or `--power10-stubs=no` on lld. `--power10-stubs=` 
will override the other option. `--power10-stubs=auto` also exists to use the 
default behaviour (ie allow Power10 instructions in stubs).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94627

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Options.td
  lld/ELF/Thunks.cpp
  lld/test/ELF/ppc64-call-reach.s
  lld/test/ELF/ppc64-long-branch-localentry-offset.s
  lld/test/ELF/ppc64-long-branch-pi.s
  lld/test/ELF/ppc64-long-branch-rel14.s
  lld/test/ELF/ppc64-long-branch.s
  lld/test/ELF/ppc64-pcrel-call-to-extern.s
  lld/test/ELF/ppc64-pcrel-call-to-toc.s
  lld/test/ELF/ppc64-plt-stub-compatible.s
  lld/test/ELF/ppc64-tls-pcrel-gd.s
  lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
  lld/test/ELF/ppc64-toc-call-to-pcrel.s
  llvm/include/llvm/Object/ELF.h

Index: llvm/include/llvm/Object/ELF.h
===
--- llvm/include/llvm/Object/ELF.h
+++ llvm/include/llvm/Object/ELF.h
@@ -50,7 +50,11 @@
 
 enum PPCInstrMasks : uint64_t {
   PADDI_R12_NO_DISP = 0x06103980,
+  ADDIS_R12_NO_DISP = 0x3D80,
+  ADDI_R12_TO_R12_NO_DISP = 0x398C,
   PLD_R12_NO_DISP = 0x0410E580,
+  LD_R12_NO_DISP = 0xE980,
+  LD_R12_TO_R12_NO_DISP = 0xE98C,
   MTCTR_R12 = 0x7D8903A6,
   BCTR = 0x4E800420,
 };
Index: lld/test/ELF/ppc64-toc-call-to-pcrel.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel.s
@@ -14,7 +14,13 @@
 # RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
 # RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t | FileCheck %s
 
-# The point of this test is to make sure that when a function with TOC access
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
+# RUN: ld.lld -T %t.script %t.o -o %t --no-power10-stubs
+# RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t \
+# RUN: | FileCheck %s
+
+## 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.
 
 # SYMBOL: Symbol table '.symtab' contains 7 entries
Index: lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
===
--- lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
+++ lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
@@ -16,10 +16,10 @@
 # SYM-NEXT: 2: 20020008 0 NOTYPE  LOCAL  DEFAULT []   3 caller
 # SYM-NEXT: 3: 10020008 0 NOTYPE  LOCAL  DEFAULT  2 caller_close
 # SYM-NEXT: 4: 000520020008 0 NOTYPE  LOCAL  DEFAULT  4 caller_far
-# SYM-NEXT: 5: 000520028038 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
+# SYM-NEXT: 5: 000520028040 0 NOTYPE  LOCAL  HIDDEN   6 .TOC.
 # SYM-NEXT: 6: 10020020 8 FUNCLOCAL  DEFAULT  2 __toc_save_callee
-# SYM-NEXT: 7: 2002002020 FUNCLOCAL  DEFAULT  3 __toc_save_callee
-# SYM-NEXT: 8: 00052002002020 FUNCLOCAL  DEFAULT  4 __toc_save_callee
+# SYM-NEXT: 7: 2002002032 FUNCLOCAL  DEFAULT  3 __toc_save_callee
+# SYM-NEXT: 8: 00052002002032 FUNCLOCAL  DEFAULT  4 __toc_save_callee
 
 #--- lts
 PHDRS {
Index: lld/test/ELF/ppc64-tls-pcrel-gd.s
===
--- lld/test/ELF/ppc64-tls-pcrel-gd.s
+++ lld/test/ELF/ppc64-tls-pcrel-gd.s
@@ -42,10 +42,10 @@
 #--- asm
 
 # GD-RELOC: Relocation section '.rela.dyn' at offset 0x100b8 contains 4 entries:
-# GD-RELOC: 01001160  00020044 R_PPC64_DTPMOD64    x + 0
-# GD-RELOC: 01001168  0002004e R_PPC64_DTPREL64    x + 0
-# GD-RELOC: 01001170  00030044 R_PPC64_DTPMOD64    y + 0
-# GD-RELOC: 01001178  0003004e R_PPC64_DTPREL64    y + 0
+# GD-RELOC: 01001170  00020044 R_PPC64_DTPMOD64    x + 0
+# GD-RELOC: 01001178  0002004e R_PPC64_DTPREL64    x + 0
+# GD-RELOC: 01001180  00030044 R_PPC64_DTPMOD64    y + 0
+# GD-RELOC: 01001188  0003004e R_PPC64_DTPREL64    y + 0
 
 # GD-SYM:   Symbol table '.dynsym' contains 4 entries:
 # GD-SYM: