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

[Mehdi Amini via llvm-commits]

Sent from my iPhone

> On Nov 19, 2015, at 9:24 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> 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.
> 

Good point removing totally the assert should be fine then.

Mehdi



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