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

[Michael Kruse via llvm-commits]

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?


>>
>> +             (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.

Michael