[llvm] r290683 - [NewGVN] Simplyfy loop NFC

Sean Silva via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 29 11:10:19 PST 2016


On Thu, Dec 29, 2016 at 1:23 AM, Piotr Padlewski <piotr.padlewski at gmail.com>
wrote:

> CurrIter have only one use.
>
>
> auto CurrIter = MI;
> ++MI;
> Value *Member = *CurrIter;
>
> I can rewrite beginning of loop as
> Value *Member = *MI++;
>
> Which makes it trivial that it is range for loop.
>
> Of course I am not 100% sure, operator* and operator++ might do some weird
> things here, but I assumed they are sane.
> Tests also passed
>


The pattern I'm thinking of is something like iterating the instructions in
a basic block, then calling eraseFromParent on the current Instruction,
which will nonetheless corrupt the list even if the iterator itself has
only one use. That's not the case here though. This particular code is safe
because the mutation of the thing being iterated is done after the loop
(the `CC->Members.swap(MembersLeft);` call); i.e. a new container is
allocated and then they are swapped at the end, instead of mutating in
place.

-- Sean Silva


>
>
> 2016-12-29 2:20 GMT+01:00 Sean Silva <chisophugis at gmail.com>:
>
>>
>>
>> On Wed, Dec 28, 2016 at 11:42 AM, Piotr Padlewski via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: prazek
>>> Date: Wed Dec 28 13:42:49 2016
>>> New Revision: 290683
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=290683&view=rev
>>> Log:
>>> [NewGVN] Simplyfy loop NFC
>>>
>>> Modified:
>>>     llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
>>>
>>> Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>>> s/Scalar/NewGVN.cpp?rev=290683&r1=290682&r2=290683&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Wed Dec 28 13:42:49 2016
>>> @@ -1915,10 +1915,7 @@ bool NewGVN::eliminateInstructions(Funct
>>>
>>>      // Cleanup the congruence class.
>>>      SmallPtrSet<Value *, 4> MembersLeft;
>>> -    for (auto MI = CC->Members.begin(), ME = CC->Members.end(); MI !=
>>> ME;) {
>>> -      auto CurrIter = MI;
>>> -      ++MI;
>>> -      Value *Member = *CurrIter;
>>> +    for (Value * Member : CC->Members) {
>>>
>>
>> Is this correct? Usually this kind of loop where the iterator is
>> incremented at the start of the loop will invalidate the iterator somewhere
>> later, so rewriting it to be a range for (or a simple loop with the
>> increment inside the `for` header where it usually goes) will introduce a
>> nasty bug.
>>
>> I haven't looked closely enough in this particular case, but I'd
>> definitely like to see a bit more justification (ideally in the commit
>> message in the future) for the safety of this refactoring when dealing with
>> this kind of "increment at the beginning" loops.
>>
>> -- Sean Silva
>>
>>
>>>        if (Member->getType()->isVoidTy()) {
>>>          MembersLeft.insert(Member);
>>>          continue;
>>>
>>>
>>> _______________________________________________
>>> 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/20161229/521ce834/attachment.html>


More information about the llvm-commits mailing list