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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Nov-19, at 08:02, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> 
> 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>`

I don't see the context for this comment, but if you're suggesting
adding an `assert(isa<Ty>())` before a `cast<Ty>()` (that's what
the comment sounds like), I disagree.  `cast<Ty>()` already asserts
on `isa<Ty>()`.  There's no reason to clutter the code on the client
side.

> 
> ================
> 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
> 
> 
>