This is an archive of the discontinued LLVM Phabricator instance.

[ELF2] - Implemented optimizations for @tlsld and @tlsgd
ClosedPublic

Authored by grimar on Nov 20 2015, 5:04 AM.

Details

Summary

Implements @tlsld (LD to LE) and @tlsgd (GD to LE) optimizations.

Patch does not implement the GD->IE case for @tlsgd, I will implement it in separate patch.
This one partially intersects with http://reviews.llvm.org/D14713.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 40768.Nov 20 2015, 5:04 AM
grimar retitled this revision from to [ELF2] - Implemented optimizations for @tlsld and @tlsgd (GD->LE).
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar retitled this revision from [ELF2] - Implemented optimizations for @tlsld and @tlsgd (GD->LE) to [ELF2] - Implemented optimizations for @tlsld and @tlsgd.
grimar added subscribers: llvm-commits, grimar.
grimar updated this revision to Diff 40771.Nov 20 2015, 5:31 AM

Updated test

grimar updated this revision to Diff 40777.Nov 20 2015, 6:32 AM

Corrected formatting issues.

ruiu added inline comments.Nov 20 2015, 11:26 AM
ELF/InputSection.cpp
103 ↗(On Diff #40777)

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;
  ...
}
114 ↗(On Diff #40777)

Instead of adding this condition, can you move this

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

block above this line?

ELF/Target.cpp
366 ↗(On Diff #40777)
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.)

377–378 ↗(On Diff #40777)

What are these instructions?

385 ↗(On Diff #40777)

Do not put a space before '!'.

410–411 ↗(On Diff #40777)

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?

grimar added inline comments.Nov 20 2015, 12:17 PM
ELF/InputSection.cpp
103 ↗(On Diff #40777)

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;
114 ↗(On Diff #40777)

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())

ELF/Target.cpp
366 ↗(On Diff #40777)

Ok

377–378 ↗(On Diff #40777)

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.

385 ↗(On Diff #40777)

Ok

410–411 ↗(On Diff #40777)

Yes, BufStart is not needed then. Lets do that.

grimar updated this revision to Diff 40906.Nov 23 2015, 2:13 AM

Review comments addressed.

ELF/InputSection.cpp
114 ↗(On Diff #40777)

My suggestion to remake function fill not work either because Body still required for Global Dynamic optimized check. Leaved that code as is.

ELF/Target.cpp
385 ↗(On Diff #40777)

Removed the check.

ruiu added inline comments.Nov 23 2015, 11:19 AM
ELF/InputSection.cpp
103 ↗(On Diff #40906)

That's too subtle. Is there any way to avoid that?

114 ↗(On Diff #40906)

Remove () from (!Target->isTlsOptimized(...))

134 ↗(On Diff #40906)

Ditto

grimar added inline comments.Nov 24 2015, 2:00 AM
ELF/InputSection.cpp
103 ↗(On Diff #40906)

gold implements that via having and setting bool flag skip_call_tls_get_addr_ that skips the relocation if true. bfd uses the approach similiar to what I used. With using flag that could be done more strict I guess (we can check relocation type and symbol name). But I think there is no chance to avoid that at all. That relocation exist but should not be proccessed.

ruiu added inline comments.Nov 24 2015, 4:00 PM
ELF/InputSection.cpp
103 ↗(On Diff #40906)

It is okay to update the index in side the loop. What I meant by too subtle is that it is not obvious that "I" may be updated by relocateTlsOptimize. It is passed by a reference that makes it almost impossible to imagine that case.

How about this. Change the signature of relocateTlsOptimize so that it returns an integer. If the next entry needs to be skipped, it returns 1. Otherwise 0. You can add that return value to I.

grimar updated this revision to Diff 41141.Nov 25 2015, 7:25 AM

Review comments addressed.

ELF/InputSection.cpp
103 ↗(On Diff #40906)

Well I cant say I like this, but I dont know much better solution atm.

114 ↗(On Diff #40906)

Done.

134 ↗(On Diff #40906)

Done.

ruiu added inline comments.Nov 25 2015, 10:13 AM
ELF/InputSection.cpp
142 ↗(On Diff #41141)

Please add a comment here.

// By optimizing TLS relocations, it is sometimes needed to skip relocations
// that immediately follow TLS relocations. This function knows how many slots
// we need to skip.
ELF/Target.cpp
469 ↗(On Diff #41141)

Please do not use int8_t just because it returns a small integer. If no specific size is in mind, use int.

469 ↗(On Diff #41141)

Add a comment

// This function applies a TLS relocation with an optimization as described
// in the Ulrich's document. As a result of rewriting instructions at the relocation
// target, relocations immediately follow the TLS relocation (which would be applied
// to rewritten instructions) need to be skipped.
// This function returns a number of relocations that need to be skipped.
472–483 ↗(On Diff #41141)
switch (Type) {
case R_X86_64_GOTTPOFF:
  relocateTlsIeToLe(...);
  return 0;
case R_X86_64_TLSLD:
  relocateTlsLdToLe(...);
  return 1;
case ...
}
grimar updated this revision to Diff 41176.Nov 25 2015, 1:23 PM

Review comments addressed.

ELF/InputSection.cpp
142 ↗(On Diff #41141)

Done.

ELF/Target.cpp
469 ↗(On Diff #41141)

Done.

469 ↗(On Diff #41141)

Done.

472–483 ↗(On Diff #41141)

Done.

ruiu accepted this revision.Nov 25 2015, 1:42 PM
ruiu edited edge metadata.

LGTM

ELF/Target.cpp
491 ↗(On Diff #41176)

need to be -> may have to be

This revision is now accepted and ready to land.Nov 25 2015, 1:42 PM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Nov 25 2015, 1:49 PM
ELF/Target.cpp
491 ↗(On Diff #41176)

Done.