[PATCH] D15068: ScopInfo: Replace while/iterator construct with std::remove_if

[David Blaikie via llvm-commits]

On Mon, Nov 30, 2015 at 1:51 AM, Michael Kruse <llvm at meinersbur.de> wrote:

> 2015-11-30 6:53 GMT+01:00 David Blaikie <dblaikie at gmail.com>:
> >
> >
> > On Sun, Nov 29, 2015 at 9:36 PM, Tobias Grosser via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> grosser created this revision.
> >> grosser added reviewers: jdoerfert, Meinersbur.
> >> grosser added subscribers: llvm-commits, pollydev.
> >>
> >> The use of C++'s high-level iterator functionality instead of two while
> >> loops
> >> and explicit iterator handling improves readability of this code.
> >>
> >> This passes LNT -polly-process-unprofitable for me.
> >>
> >> Proposed-by: Michael Kruse <llvm at meinersbur.de>
> >>
> >> http://reviews.llvm.org/D15068
> >>
> >> Files:
> >>   lib/Analysis/ScopInfo.cpp
> >>
> >> Index: lib/Analysis/ScopInfo.cpp
> >> ===================================================================
> >> --- lib/Analysis/ScopInfo.cpp
> >> +++ lib/Analysis/ScopInfo.cpp
> >> @@ -1464,28 +1464,18 @@
> >>  void ScopStmt::dump() const { print(dbgs()); }
> >>
> >>  void ScopStmt::removeMemoryAccesses(MemoryAccessList &InvMAs) {
> >> -
> >> -  // Remove all memory accesses in @p InvMAs from this statement
> together
> >> -  // with all scalar accesses that were caused by them. The tricky
> >> iteration
> >> -  // order uses is needed because the MemAccs is a vector and the order
> >> in
> >> -  // which the accesses of each memory access list (MAL) are stored in
> >> this
> >> -  // vector is reversed.
> >> +  // Remove all memory accesses in @p InvMAs from this statement
> >> +  // together with all scalar accesses that were caused by them.
> >>    for (MemoryAccess *MA : InvMAs) {
> >> -    auto &MAL = *lookupAccessesFor(MA->getAccessInstruction());
> >> -    MAL.reverse();
> >> -
> >> -    auto MALIt = MAL.begin();
> >> -    auto MALEnd = MAL.end();
> >> -    auto MemAccsIt = MemAccs.begin();
> >> -    while (MALIt != MALEnd) {
> >> -      while (*MemAccsIt != *MALIt)
> >> -        MemAccsIt++;
> >> -
> >> -      MALIt++;
> >> -      MemAccs.erase(MemAccsIt);
> >> -    }
> >> -
> >> +    auto Predicate = [MA](MemoryAccess *Acc) -> bool {
> >
> >
> > Just capture everything by ref here ^ ("[&]") since the lambda is being
> used
> > in the scope (it's not leaking out via std::function, etc) so everything
> > will be valid & it should just be treated like a normal scope.
>
> Is this the preferred style or is there some other reason?
>

Just stylistic - open to discussion (the specific ins and outs of C++11
style both across the field and across LLVM's codebase are still in flux,
for sure).

I've mentioned this on one or two other threads, but my feeling is that if
the lambda is only used within the lexical scope (& especially if it's only
used within a single full expression as with most lambda usage in standard
and standard-esque algorithms) then it should work like another nested
lexical scope: simply capture everything implicitly by reference. Anything
else and it's likely to be a surprise (especially a capture by value
default, but ever just explicit captures might cause undue
friction/maintenance pain)


>
>
> >>
> >> +             (Acc->isWrite() &&
> >> +              Acc->getAccessInstruction() ==
> MA->getAccessInstruction());
> >> +    };
> >> +    MemAccs.erase(std::remove_if(MemAccs.begin(), MemAccs.end(),
> >> Predicate),
> >
> >
> > Probably just define the lambda directly inline without giving it a name?
>
> From where Tobias extracted the code from
> (http://reviews.llvm.org/D13676), the lambda was used twice. D13676 is
> still under review.
>

ah, thanks for the context


>
> Michael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151130/da2b278a/attachment.html>