[PATCH] D54720: [PPC64] toc-indirect to toc-relative relaxation.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments/suggestions.



================
Comment at: ELF/Arch/PPC64.cpp:110
+  if (Index >= Relas.size()) {
+    warn("Invalid offset " + Twine(Offset) + " into the .toc section in " +
+         toString(TocSec->File));
----------------
Warnings and error messages should start from the lowercase.


================
Comment at: ELF/Arch/PPC64.cpp:120
+  if (Offset != Rela.r_offset) {
+    warn("Offset mismatch on toc relaxation in " + toString(TocSec->File) +
+         " expected: " + Twine(Offset) + " but found: " + Twine(Rela.r_offset));
----------------
The same.


================
Comment at: ELF/Arch/PPC64.cpp:127
+  return std::make_pair<Defined *, int64_t>(
+      dyn_cast_or_null<Defined>(&IndirectSym), getAddend<ELFT>(Rela));
+}
----------------
You can write shorter:

```
  return {dyn_cast_or_null<Defined>(&IndirectSym), getAddend<ELFT>(Rela)};
```


================
Comment at: ELF/Arch/PPC64.cpp:137
+  if (!DefSym || !DefSym->isSection() ||
+      !(DefSym->Section->Name.compare(".toc") == 0))
+    return std::make_pair<Defined *, int64_t>(nullptr, 0);
----------------
Why not `DefSym->Section->Name != ".toc"` ?


================
Comment at: ELF/Arch/PPC64.cpp:138
+      !(DefSym->Section->Name.compare(".toc") == 0))
+    return std::make_pair<Defined *, int64_t>(nullptr, 0);
+
----------------
`return {};`


================
Comment at: ELF/Arch/PPC64.cpp:141
+  if (Rel.Addend < 0)
+    return std::make_pair<Defined *, int64_t>(nullptr, 0);
+
----------------
`return {};`


================
Comment at: ELF/Arch/PPC64.cpp:155
+  if (!Config->TocOptimize)
+    return false;
+
----------------
So maybe you should never reach here if `!Config->TocOptimize`?
Seems you can do something like the following in `getRelExpr`:

```
  case R_PPC64_TOC16_HA:
  case R_PPC64_TOC16_LO_DS:
    return Config->TocOptimize ? R_PPC64_RELAX_TOC : R_XXXX;
```


================
Comment at: ELF/Arch/PPC64.cpp:157
+
+  std::pair<Defined *, int64_t> RelaxTo = relaxTo(Rel);
+  // If the first member is a nullptr then we didn't find a symbol
----------------
`RelaxTo.first`/`RelaxTo.second` are a bit hard to follow and read. Lets do:

```
  Defined *D;
  int64_t Addend;
  std::tie(D, Addend) = relaxTo(Rel);
```


================
Comment at: ELF/InputSection.cpp:969
+      if (!tryRelaxTocPPC64(File, Type, Rel, Expr, BufLoc, AddrLoc))
+        Target->relocateOne(BufLoc, Type, TargetVA);
       break;
----------------
sfertile wrote:
> grimar wrote:
> > I think we usually reach here (`relocateAlloc`) when we already know will we do the relaxation or not.
> > I.e. for example on x86_64 for `R_RELAX_GOT_PC` we just call `Target->relaxGot` because check in the `adjustRelaxExpr` the conditions for relaxing. And here then we either have `R_RELAX_GOT_PC` which name contains `RELAX` and means we *will do* the relaxation or we would have another R_* expression if we are not going to relax.
> > 
> > So I think you should not have a `R_PPC64_RELAX_TOC` expression here if you are not going to relax. I think you should check the conditions somewhere earlier. Also, the platform-specific code can live into own file (like ELF\Arch\PPC64.cpp) then. I think you can put all the functions you added there.
> > 
> > 
> The got-indirect to toc-relative relaxation is unique in this regard because we can't decide if we can relax until the data segment has been fully built. During scanRelocs we could see if the indirection is to a defined symbol, but that is only enough to know that the relocation is// potentially //relax-able. We still need the offset from the TOC base pointer to the symbols definition to fit in a signed 32-bit offset (which isn't guaranteed in the large code model), and scanRelocs is too early to determine that.  I could add the relax checking earlier than here but that would involve doing the same scan over the .toc sections relocations and replacing those that are indeed relaxable. I decided not to do that in the spirit of keeping it simple.
> 
> I like the idea of moving the platform specific code into PPC64.cpp, and will update the patch with that change, but I think here is the right place to perform the relaxation.
> 
OK, thanks for the explanation.


================
Comment at: ELF/Target.h:172
+// this function performs the relaxation and returns true. Returns false
+// otherwise.
+bool tryRelaxTocPPC64(RelType Type, const Relocation &Rel,
----------------
I think `Returns false otherwise` part is an obvious continuation and hence excessive.


================
Comment at: test/ELF/ppc64-got-indirect.s:8
+# RUN: ld.lld %t.o %t1.so -o %t2
 # RUN: llvm-objdump -D %t2 | FileCheck %s --check-prefix=CHECK-LE
 # RUN: llvm-objdump -D %t2 | FileCheck %s
----------------
You can use

```
--check-prefixes=CHECK,CHECK-LE
```


================
Comment at: test/ELF/ppc64-toc-relax-jumptable.s:36
+
+        .text
+        .global _start
----------------
Can you remove the excessive indentations please?


================
Comment at: test/ELF/ppc64-toc-relax.s:62
+
+#CHECK-LABEL: can_not_relax:
+# CHECK-NEXT: 10010018:
----------------
```
# CHECK-LABEL: ...
```


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54720