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

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 20 12:18:00 PST 2015


grimar 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);
----------------
ruiu wrote:
> 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;
>     ...
>   }
> 
I need that to be able to skip relocations, not for counting ID. Please see below next parts of code:

```
// The next relocation should be against __tls_get_addr, so skip it.
  ++RelNdx;
```


================
Comment at: ELF/InputSection.cpp:114
@@ -112,1 +113,3 @@
+    if (Target->isTlsLocalDynamicReloc(Type) &&
+        (!Target->isTlsOptimized(Type, nullptr))) {
       Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
----------------
ruiu wrote:
> Instead of adding this condition, can you move this
> 
>   if (Body.isTLS() && Target->isTlsOptimized(Type, &Body)) {
>     ....
>   }
> 
> block above this line?
Thats probably will not work.
I need to get the Body.

```
SymbolBody &Body = *File->getSymbolBody(SymIndex)->repl();
```
To do that we need to check/handle locals first that dont have it. That is done below:
 
```
   if (SymIndex < SymTab->sh_info) {
...
    }
```

What about if I remake isTlsLocalDynamicReloc() to isNoOptTlsLocalDynamicReloc() (NotOptimized + TlsLocalDynamicReloc) ? That should work everywhere in code.
(and the same for isTlsGlobalDynamicReloc())

================
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));
----------------
ruiu wrote:
>   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.)
Ok

================
Comment at: ELF/Target.cpp:377-378
@@ +376,4 @@
+// Is converted to:
+//  .word 0x6666
+//  .byte 0x66
+//  mov %fs:0,%rax
----------------
ruiu wrote:
> What are these instructions?
Its mentioned on p.23 of http://www.akkadia.org/drepper/tls.pdf. That is just prefixes that used to fill the whole sequence. For some reason not no-ops operations should be used.
p.65 contains the info about this one optimization where is told to do in that way either.

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

================
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[] = {
----------------
ruiu wrote:
> 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?
Yes, BufStart is not needed then. Lets do that.


http://reviews.llvm.org/D14870





More information about the llvm-commits mailing list