[PATCH] D15779: [ELF] - Implemented optimization for R_X86_64_GOTPCREL relocation.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Target.cpp:698
@@ +697,3 @@
+// R_X86_64_GOTPCREL if we perform additional check of instructions. Both gold
+// and bfd do the same.
+bool X86_64TargetInfo::canOptimizeGotPcRel(uint32_t Type, const SymbolBody &S,
----------------
rafael wrote:
> Update the comment to say that we can do mov -> lea with R_X86_64_GOTPCREL, but we need R_X86_64_GOTPCRELX for the other optimizations
Ok, I can do that. 
I just supposed that since we are not going to implement any other ones before R_X86_64_GOTPCRELX then such comment is excessive.
Current one just describes what we do, not sure if we want to list everything we will not do :)

================
Comment at: ELF/Target.cpp:702
@@ +701,3 @@
+                                           uint64_t Off) const {
+  if (Type != R_X86_64_GOTPCREL || Off <= 2 || isGnuIFunc<ELF64LE>(S))
+    return false;
----------------
rafael wrote:
> Could canBePreempted take care of the ifunc check?
I would not do it.
For example we have the next code currently:

```
if (!CanBePreempted && Body && isGnuIFunc<ELFT>(*Body))
      Reloc = Target->getIRelativeReloc();
```

Currently we implemented ifunc for static linking case. But afaik gold/bfd supports dynamic case for which real CBP makes sence. And I am not sure we should cover such special type under clear for understanding canBePreempted() name.

================
Comment at: ELF/Target.cpp:955
@@ -906,1 +954,3 @@
   case R_X86_64_GOTPCREL:
+    if (S && canOptimizeGotPcRel(Type, *S, Loc, (uint64_t)-1))
+      optimizeGotPcRel(Loc);
----------------
rafael wrote:
> You are passing -1 in here because the offset is not available, correct?
> Is there any point in ever passing the offset if we cannot pass it here?
I cannot pass it here, but I still do the check for that place inside canOptimizeGotPcRel().
I just assume that there is not possible to have negative buffer overflow from here.
My point was: since we dont have  R_X86_64_GOTPCRELX yet its better to do all possible additional checks. After implementation of R_X86_64_GOTPCRELX we probably can remove it (like we dont check overflows for TLS relaxations, assuming inputs are correct).

Also I am not sure will we still perform relaxation for raw R_X86_64_GOTPCREL then or not (I quess better would be to stop support to be consistent with ABI, but if both gold/bfd do that then it is still safe I think).


http://reviews.llvm.org/D15779