[PATCH] Implement ADRP CSE for global symbols

[Tim Northover]

  Hi Jiangning,

  This sounds like a good idea. I've got a couple of smaller comments:


================
Comment at: test/CodeGen/AArch64/global_merge_2.ll:1
@@ +1,2 @@
+; RUN: llc < %s -mtriple=aarch64-none-linux-gnu -global-merge-on-external=true | FileCheck %s
+
----------------
And speaking of tests, perhaps this would be a good time to promote GlobalMerge to something that can be run through opt and use that to do the testing.

================
Comment at: lib/Transforms/Scalar/GlobalMerge.cpp:287-290
@@ -248,2 +286,6 @@
+    // Merge is safe for "normal" internal or external globals only
+    if (!((EnableGlobalMergeOnExternal && I->hasExternalLinkage())
+          || I->hasInternalLinkage())
+        || I->isDeclaration() || I->isThreadLocal() || I->hasSection())
       continue;
 
----------------
This might be clearer split up. I know my brain starts to melt when I see massive nested conditionals like this. Perhaps linkage conditions vs intrinsic blockers?

================
Comment at: lib/IR/Globals.cpp:277-279
@@ +276,5 @@
+  while (C) {
+    if (const GlobalAlias *GA = dyn_cast<GlobalAlias>(C)) {
+      C = GA->getAliasee();
+    } else if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
+      if (CE->getOpcode() == Instruction::GetElementPtr) {
----------------
Are these clauses tested? I don't see any aliases or GEPs in your examples. I also vaguely remember Rafael saying that a GEP-based alias wasn't intended to be supported, though I wouldn't swear to it (and I'm fuzzy on exactly why it's a bad idea).


http://reviews.llvm.org/D3432