[PATCH] D14819: [GlobalOpt] Localize some globals that have non-instruction users

[Mehdi AMINI via llvm-commits]

joker.eph accepted this revision.
joker.eph added a comment.
This revision is now accepted and ready to land.

LGTM with some comments.


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1845
@@ +1844,3 @@
+  for (auto *U : Users) {
+    SmallVector<Value*,4> UUsers;
+    for (auto *UU : U->users())
----------------
Nit:  A common pattern is to put UUsers out of the loop and call clear at the end of each iteration. So that if you cross the limit of 4 multiple times you won't pay the dynamic allocation (and the copy) again.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1849
@@ +1848,3 @@
+    for (auto *UU : UUsers) {
+      assert(!isa<Constant>(UU));
+
----------------
Minor suggestion: I'd rather read `assert(isa<Instruction>)` before `cast<Instruction>`

================
Comment at: test/Transforms/GlobalOpt/localize-constexpr.ll:9
@@ +8,3 @@
+; CHECK-NOT: @G
+; CHECK: }
+  store i32 42, i32* @G
----------------
The last check is spurious. The next CHECK-LABEL should be enough I think.


Repository:
  rL LLVM

http://reviews.llvm.org/D14819