[PATCH] [RFC][PATCH] Minor opt to access pointers to globals via pcrel GOT entries

[Bruno Cardoso Lopes]

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:340
@@ -338,1 +339,3 @@
 
+    // Skip the emission of global proxies.
+    if (GlobalGOTEquivs.count(getSymbol(GV)))
----------------
rafael wrote:
> Say that emitGlobalGOTEquivs will emit them if needed.
--------
OK

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:918
@@ +917,3 @@
+  if (!GV->hasUnnamedAddr() || !GV->hasInitializer() || !GV->isConstant() ||
+      !GV->isDiscardableIfUnused() || !GV->getType()->isPointerTy() ||
+      !dyn_cast<GlobalValue>(GV->getOperand(0)))
----------------
rafael wrote:
> All global Variables have pointer type.
--------
OK

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:919
@@ +918,3 @@
+      !GV->isDiscardableIfUnused() || !GV->getType()->isPointerTy() ||
+      !dyn_cast<GlobalValue>(GV->getOperand(0)))
+    return false;
----------------
rafael wrote:
> s/dyn_cast/isa/
> 
--------
OK

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:932
@@ +931,3 @@
+// globals variable act like a global variable "proxy", or GOT equivalents,
+// i.e., it's only used to hold the address of the latter. One (very) minor
+// optimization is to replace accesses to these proxies by using the GOT entry
----------------
rafael wrote:
> not sure there is a point is saying it is "(very) minor" :-)
--------
Ops, forgot to fix this one :-)

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:937
@@ +936,3 @@
+// finally replace references to them by pc relative accesses to GOT entries.
+void AsmPrinter::computeGlobalGOTEquivs(Module &M) {
+  if (!getObjFileLowering().supportIndirectSymViaGOTPCRel())
----------------
rafael wrote:
> High level question: Do we need to do the early pass over all variables?
> 
> Couldn't emitGlobalConstantImpl do the check and add "got equivalent candidates"?
--------
The problem of not doing an early pass is because we can't rely on the global declaration order - if we hit the 'got equivalent user' before keeping track of the 'global equivalent', we have to find the global variable for 'global equivalent' after its MCSymbol, because at that point we already lost the GlobalVariable reference. One could argue that we can find this GlobalVariable again by scanning the original cstexpr where we got the MCExpr from, but this also would require complex logic to handle all different types of cstexpr that could lead to a relocatable expression. Gonna add a comment about that.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2081
@@ +2080,3 @@
+  int64_t BaseSymOff = -Offset;
+  if (BaseSym != &MV.getSymB()->getSymbol() || MV.getConstant() != BaseSymOff)
+    return;
----------------
rafael wrote:
> I am not sure I understand why the  MV.getConstant() != BaseSymOff restriction is necessary.
> 
> If we have
> -------------------------
> .globl  _proxy
> _proxy:
>  .quad   _foo
> 
>  .globl  _delta
> _delta:
>  .long   _proxy-_delta + 42
> ------------------------------------
> 
> It can be converted to
> -----------------------------
> .globl  _delta
> _delta:
>  .long   _foo at GOTPCREL + 42
> ----------------------------
> 
> no? It works on ELF at least.
> 
> 
> 
Note that the reason why BaseSymOff exists is to ensure that "." is properly matched, i.e., we need to know that the symbol we are subtracting from is indeed the "PC" location. Take your example:

  delta:
    .long (.Lproxy-delta)+42
    .size delta, 4

Here, "." == "delta" and BaseSymOff == 0, since we're emitting the first and only element of delta. Now, if we're emitting the GOTPCREL into, let's say, the second element of an array where each element has 4 bytes, we need "." == "delta+4" and BaseSymOff == 4, since we want to be sure we're dealing with a pc relative location:

  delta:
    .long 33
    .long (.Lproxy-(delta+4))+42 ==> .long (.Lproxy-delta)+38
    .size delta, 8

That said, the restriction isn't necessary as long as we have BaseSymOff contained into the final offset value. I didn't think of that use before although it's valid and looks like it could be (?) used by front-end writers to do all sort of black magic.

It's a small change, gonna include that in the patch and write a test for it using a cstexpr that also adds one more constant.

================
Comment at: test/MC/X86/cstexpr-gotpcrel.ll:1
@@ +1,2 @@
+; RUN: llc -mtriple=x86_64-apple-darwin %s -o - | FileCheck %s
+
----------------
rafael wrote:
> It would be nice to add code coverage on the cases where the optimization fails (use in instructions for example).
-------------
Ok

http://reviews.llvm.org/D6922

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/