[PATCH] D49237: [PPC64] Optimize redundant instructions using R_PPC64_TOC16_HA in nop

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:132
+  uint8_t OpCode = getPrimaryOpCode(Encoding);
+  switch (OpCode) {
+  default:
----------------
This variable is used only once. You can eliminate it.


================
Comment at: ELF/Arch/PPC64.cpp:560
+static bool isTocRelType(RelType Type) {
+  if (Type == R_PPC64_TOC16_HA || Type == R_PPC64_TOC16_LO_DS ||
+      Type == R_PPC64_TOC16_LO)
----------------
Just return the result of the boolean expression instead of `if (e) return true; return false;`


================
Comment at: ELF/Arch/PPC64.cpp:633
+        error(getErrorLocation(Loc) +
+              "Can't toc-optimize an update instruction: 0x" +
+              Twine::utohexstr(Instr));
----------------
Can't -> can't


================
Comment at: ELF/Arch/PPC64.cpp:634
+              "Can't toc-optimize an update instruction: 0x" +
+              Twine::utohexstr(Instr));
+      }
----------------
We have llvm::utohexstr, so I believe you can omit `Twine::`.


================
Comment at: ELF/Driver.cpp:287-288
+
+    if (Args.hasArg(OPT_no_toc_optimize))
+      error("--no-toc-optimize is only supported on the PowerPC64 target.");
+  }
----------------
It is indeed true that --no-toc-optimize is PPC64 only, but in other test patterns we don't care about that kind of negative flags. I'd check if `Config->TocOptimize` is true.


================
Comment at: ELF/Options.td:319-320
+defm toc_optimize : B<"toc-optimize",
+                      "(PowerPC64) Enable toc related optimizations (default)",
+                      "(PowerPC64) Disable toc related optimizations">;
+
----------------
Indentation.

Shouldn't TOC be all-caps?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49237