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

[Rafael Ávila de Espíndola]

This seems to be implemented a bit lower than I was expecting.

The idea is that is is an codegen optimization, right? When given a .s, we should just produce a .o that matches it.

If that is the case, why is this ever looking at symbols and MCExpr? It should be happening at a higher lever (GlobalValues and ConstantExpr).


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/AsmPrinter.h:105
@@ +104,3 @@
+  typedef std::pair<const GlobalVariable *, unsigned> GblProxyUsePair;
+  DenseMap<const MCSymbol *, GblProxyUsePair> GlobalProxies;
+
----------------
Proxy seems like a new term.

I would probably go with *GOT*. GOTEquivalent maybe?

================
Comment at: include/llvm/CodeGen/AsmPrinter.h:260
@@ +259,3 @@
+  /// finally replace references to them by pc relative accesses to GOT entries.
+  void GetGlobalProxies(Module &M);
+
----------------
Start functions with a lowercase.

================
Comment at: include/llvm/Target/TargetLoweringObjectFile.h:156
@@ +155,3 @@
+  /// through another symbol, by accessing the later via a GOT entry instead?
+  virtual bool supportIndirectSymViaGOTPCRel() const { return false; }
+
----------------
Instead of having a method that always return a fixed bool, make this an actual boolean and set it in the constructor.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:341
@@ +340,3 @@
+    // Skip the emission of global proxies.
+    if (getObjFileLowering().supportIndirectSymViaGOTPCRel() &&
+        GlobalProxies.count(getSymbol(GV)))
----------------
Why do you need to call support* in here? The could will be 0 if it is not supported, no?

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:893
@@ +892,3 @@
+static bool isGlobalProxyCandidate(const GlobalVariable *GV) {
+  if (GV->use_empty())
+    return false;
----------------
Why do you need to special case not having any users?

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:901
@@ +900,3 @@
+
+    const Constant *C = cast<Constant>(CE);
+    while (C && C->getNumUses() == 1 && !isa<GlobalVariable>(C))
----------------
You don't need a cast from ConstatExpr to Constant.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:905
@@ +904,3 @@
+
+    if (!isa<GlobalVariable>(C))
+      return false;
----------------
C can be null in here, no?

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:912
@@ +911,3 @@
+
+// GetGlobalProxies - Unnamed constant global variables solely contaning a
+// pointet to another globals variables are act like a global variable "proxy",
----------------
Don't repeat name in comments.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:919
@@ +918,3 @@
+// replace references to them by pc relative accesses to GOT entries.
+void AsmPrinter::GetGlobalProxies(Module &M) {
+  if (getObjFileLowering().supportIndirectSymViaGOTPCRel())
----------------
s/Get/compute/ maybe? Get sounds like the function returns them.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:929
@@ +928,3 @@
+
+    const GlobalVariable *FinalGV = dyn_cast<GlobalVariable>(G.getOperand(0));
+    if (!FinalGV)
----------------
Why not fold these checks into isGlobalProxyCandidate? 

================
Comment at: test/MC/X86/cstexpr-gotpcrel.ll:1
@@ +1,2 @@
+; RUN: llc -filetype=obj -mtriple=x86_64-apple-darwin %s -o - | llvm-readobj -r | FileCheck -check-prefix=X86-OBJ %s
+; RUN: llc -mtriple=x86_64-apple-darwin %s -o - | FileCheck -check-prefix=X86 %s
----------------
Please don't use "llc -filetype=obj". To test the object emission, use llvm-mc.

http://reviews.llvm.org/D6922

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