<div dir="ltr">Hi Guys,<div><br></div><div>Yep, agreed. I'll be committing with the change suggested by Mehdi and the removal of the assert.</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, 19 Nov 2015 at 17:36 Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
Sent from my iPhone<br>
<br>
> On Nov 19, 2015, at 9:24 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
><br>
>> On 2015-Nov-19, at 08:02, Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>> wrote:<br>
>><br>
>> joker.eph accepted this revision.<br>
>> joker.eph added a comment.<br>
>> This revision is now accepted and ready to land.<br>
>><br>
>> LGTM with some comments.<br>
>><br>
>><br>
>> ================<br>
>> Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1845<br>
>> @@ +1844,3 @@<br>
>> +  for (auto *U : Users) {<br>
>> +    SmallVector<Value*,4> UUsers;<br>
>> +    for (auto *UU : U->users())<br>
>> ----------------<br>
>> 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.<br>
>><br>
>> ================<br>
>> Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1849<br>
>> @@ +1848,3 @@<br>
>> +    for (auto *UU : UUsers) {<br>
>> +      assert(!isa<Constant>(UU));<br>
>> +<br>
>> ----------------<br>
>> Minor suggestion: I'd rather read `assert(isa<Instruction>)` before `cast<Instruction>`<br>
><br>
> I don't see the context for this comment, but if you're suggesting<br>
> adding an `assert(isa<Ty>())` before a `cast<Ty>()` (that's what<br>
> the comment sounds like), I disagree.  `cast<Ty>()` already asserts<br>
> on `isa<Ty>()`.  There's no reason to clutter the code on the client<br>
> side.<br>
><br>
<br>
Good point removing totally the assert should be fine then.<br>
<br>
Mehdi<br>
<br>
<br>
<br>
>><br>
>> ================<br>
>> Comment at: test/Transforms/GlobalOpt/localize-constexpr.ll:9<br>
>> @@ +8,3 @@<br>
>> +; CHECK-NOT: @G<br>
>> +; CHECK: }<br>
>> +  store i32 42, i32* @G<br>
>> ----------------<br>
>> The last check is spurious. The next CHECK-LABEL should be enough I think.<br>
>><br>
>><br>
>> Repository:<br>
>> rL LLVM<br>
>><br>
>> <a href="http://reviews.llvm.org/D14819" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14819</a><br>
><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>