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

[James Molloy via llvm-commits]

Hi Guys,

Yep, agreed. I'll be committing with the change suggested by Mehdi and the
removal of the assert.

James

On Thu, 19 Nov 2015 at 17:36 Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> 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
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151119/e8ae5562/attachment.html>