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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:266
+      llvm_unreachable(
+          "Expected a 'ld' for got-indirect to toc-relative relaxing");
+    Instr &= 0x03FFFFFF;
----------------
Error messages should start with a lowercase letter.

I think you should use `error()` instead of `llvm_unreachable()` because this condition can be triggered by a user. `llvm_unreachable` should be used only to report an internal error or a bug.


================
Comment at: ELF/Arch/PPC64.cpp:267-269
+    Instr &= 0x03FFFFFF;
+    Instr |= 0x38000000;
+    writeInstrFromHalf16(Loc, Instr);
----------------
I think `Instr = (Instr & 0x3FFFFFF) | 0x38000000;` is a bit more straightforward.

Or,

  writeInstrFromHalf16(Loc, (Instr & 0x3FFFFFF) | 0x38000000);

is perhaps better.


================
Comment at: ELF/Arch/PPC64.cpp:271
+    relocateOne(Loc, R_PPC64_TOC16_LO, Val);
+  } break;
+  default:
----------------
This is an unusual formatting style in lld. Please put `break` before `}`.


================
Comment at: ELF/Arch/PPC64.cpp:274
+    llvm_unreachable(
+        "Unexpected relocation type for got-indirect to toc-relative relaxing");
+  }
----------------
U -> u


================
Comment at: ELF/InputSection.cpp:867
+// Looks up the symbol and addend that a .toc indirection refers to.
+template <typename ElfTy>
+static llvm::Optional<PPC64RelaxTo>
----------------
ElfTy -> ELFT


================
Comment at: ELF/InputSection.cpp:868
+template <typename ElfTy>
+static llvm::Optional<PPC64RelaxTo>
+getSymAndAddendPPC64(InputSectionBase *TocSec, uint64_t Offset) {
----------------
Can you return `std::pair<Defined *, int64_t>` instead of `Optional<std::pair<Defined *, int64_t>>`? It looks like you can simply return `{nullptr, 0}` to indicate a null value.


================
Comment at: ELF/InputSection.cpp:870
+getSymAndAddendPPC64(InputSectionBase *TocSec, uint64_t Offset) {
+  auto Relas = TocSec->template relas<ElfTy>();
+  // Check that the Offset maps to a valid relocation.
----------------
Replace `auto` with the actual type.


================
Comment at: ELF/InputSection.cpp:882
+  // through all relocations to find the one with the correct offset.
+  auto Rela = Relas[Index];
+  if (Offset != Rela.r_offset) {
----------------
Ditto


================
Comment at: ELF/InputSection.cpp:923
+
+  auto CanRelaxTo = relaxToPPC64(Rel);
+  if (!CanRelaxTo.hasValue())
----------------
Please do not use `auto` in lld unless its actual type is obvious from right-hand side.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54720