[PATCH] D14148: [GlobalOpt] Demote globals to locals more aggressively

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1745
@@ +1744,3 @@
+  return true;
+}
+
----------------
Note: I believe this is equivalent to 
```
return !std::any_of(CGN->begin(), CGN->end(), [&] (CallGraphNode::CallRecord &CR) { return CR.second == CGN });
```

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1752
@@ +1751,3 @@
+
+bool GlobalOpt::isFunctionKnownNotToRecurse(const Function *F) {
+
----------------
Whether we go with an attribute (which I would support a priori) or not, it seems cleaner to expose the result of this through a module level analyses.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1762
@@ +1761,3 @@
+      // call itself, or (conservatively) be callable by external functions.
+      if (SCC.size() != 1 || !F || !F->hasLocalLinkage())
+        continue;
----------------
Could the check `!F` be an assert or are there legitimate cases for this?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1800
@@ +1799,3 @@
+        // alias (not must alias) with any preceding stores.
+        return false;
+      
----------------
I don't get this part, even if the load is larger than the GV you should have a must alias. Isn't the opposite case that is problematic? 

Edit: reading later I think I figured: you need to makes sure that every load can be replace by the value forwarded from another store. I'm not sure "must alias" is the best wording here.

LLVM defines: `The MustAlias response may only be returned if the two memory objects are guaranteed to always start at exactly the same location. A MustAlias response implies that the pointers compare equal.`

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1814
@@ +1813,3 @@
+      Stores.push_back(I);
+    } else {
+      return false;
----------------
No else, just return.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1834
@@ +1833,3 @@
+    if (!Resolved)
+      return false;
+  }
----------------
It would be nice if SmallVector (and other ADT) would allow to test a predicate on their content, this pattern would be less verbose. In the meantime I guess we're left with something like:

```
if(!std::any_of(Stores.begin(), Stores.end(), [&] (Instruction *S) { return DT.dominates(S, L); })
  return false;
```

If you're not allergic to the STL, or alternatively you may outline this like you did for `functionDoesNotCallItself`.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1937
@@ +1936,3 @@
+  // and this function is main (which we know is not recursive), we replace
+  // the global with a local alloca in this function.
+  //
----------------
So you moved this optimization a after all the previous one (like `TryToShrinkGlobalToBoolean`, `OptimizeOnceStoredGlobal`, ...), why?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2030
@@ -1914,1 +2029,3 @@
+      CGN->removeAllCalledFunctions();
+      delete CG->removeFunctionFromModule(CGN);
       Changed = true;
----------------
` F->eraseFromParent();` is pretty straightforward and usual, this could deserve a comment.


Repository:
  rL LLVM

http://reviews.llvm.org/D14148