[PATCH] D14870: [ELF2] - Implemented optimizations for @tlsld and @tlsgd

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputSection.cpp:103
@@ -102,1 +102,3 @@
+  for (size_t I = 0; I < Num; ++I) {
+    const RelType &RI = *(Rels.begin() + I);
     uint32_t SymIndex = RI.getSymbol(Config->Mips64EL);
----------------
It is not obvious that Rels.begin() is a randomly-accessible iterator in this context, so this is a bit alarming. Probably it is better to write

  size_t Idx = -1;
  for (const RelType &RI : Rels) {
    ++Idx;
    ...
  }


================
Comment at: ELF/InputSection.cpp:114
@@ -112,1 +113,3 @@
+    if (Target->isTlsLocalDynamicReloc(Type) &&
+        (!Target->isTlsOptimized(Type, nullptr))) {
       Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
----------------
Instead of adding this condition, can you move this

  if (Body.isTLS() && Target->isTlsOptimized(Type, &Body)) {
    ....
  }

block above this line?

================
Comment at: ELF/Target.cpp:366
@@ +365,3 @@
+    return false;
+  return (Type == R_X86_64_TLSLD) || (Type == R_X86_64_DTPOFF32) ||
+         (Type == R_X86_64_TLSGD && !canBePreempted(S, true));
----------------
  return (Type == R_X86_64_TLSLD) || (Type == R_X86_64_DTPOFF32) ||

->

  return Type == R_X86_64_TLSLD || Type == R_X86_64_DTPOFF32 ||

(We can assume that everybody knows == take higher precedence over || at least.)

================
Comment at: ELF/Target.cpp:377-378
@@ +376,4 @@
+// Is converted to:
+//  .word 0x6666
+//  .byte 0x66
+//  mov %fs:0,%rax
----------------
What are these instructions?

================
Comment at: ELF/Target.cpp:385
@@ +384,3 @@
+  if (Loc - 3 < BufStart)
+    error("Tls relocation LdToLe optimization fail, buffer overrun !");
+  const uint8_t Inst[] = {
----------------
Do not put a space before '!'.

================
Comment at: ELF/Target.cpp:410-411
@@ +409,4 @@
+                                         uint64_t SA, size_t &RelNdx) const {
+  if (Loc - 4 < BufStart || Loc + 12 > BufEnd)
+    error("Tls relocation GdToLe optimization fail, buffer overrun !");
+  const uint8_t Inst[] = {
----------------
Feels like these error checks are becoming more annoying than useful. I remember you mentioned to remove them. I agree with that. If these relocations are guaranteed to be used with only specific set of instructions, we don't always need to check for that condition (if users throw in garbage, it may output garbage).

If you remove this error check, you can remove BufStart from argument list?


http://reviews.llvm.org/D14870