[PATCH] Implement ADRP CSE for global symbols

Jiangning Liu liujiangning1 at gmail.com
Mon Apr 21 23:32:48 PDT 2014



================
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;
 
----------------
Tim Northover wrote:
> 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?
Tim, That's OK, and I've split it up in new version.

================
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) {
----------------
Tim Northover wrote:
> 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).
Tim, Yes, these clauses are tested, and the follows are just aliases to be checked.

; CHECK: y = _MergedGlobals_x+4
; CHECK: z = _MergedGlobals_x+8

Without this change, compiler would fail to run the test case global_merge_2.ll.

GEPs are from ".globl  y" and ".globl z"

i32* getelementptr inbounds ({ i32, i32, i32 }* @_MergedGlobals_x, i32 0, i32 1)
i32* getelementptr inbounds ({ i32, i32, i32 }* @_MergedGlobals_x, i32 0, i32 2)

I don't know what Rafael had said previously, can you point me that email thread if you know, then I can consider this issue.


http://reviews.llvm.org/D3432






More information about the llvm-commits mailing list