llvm dev - Apr 2015 - [LLVMdev] Why the fault?

for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I != E; ) {

    Instruction& inst = *I;

    ++I; <-- iterator should be advanced to the previous instruction

    // Happens to be an Instruction::SExt.
    // Works fine if I iterate forwards
    if (isInstructionTriviallyDead(&inst, TLI))
      inst.eraseFromParent();
  }

Sorry for the inexperienced question, but I'm confused why this works when
using a forward iterator, but fails in reverse. The iterator is moved off the
current instruction so why would erasing the current instruction cause an error?

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150401/1430f2a1/attachment.html>

ilist uses std::reverse_iterator.

reverse iterator erase  is not that simple, because
std::reverse_iterator does not return  element, but element-1, when it
is dereferenced.
(IE it lies).

In particular, the standard says the relationship between
std::reverse_iterator and the base iterator you hand it is:

&*(reverse_iterator(i)) == &*(base - 1)


If you wanted it to be unchanged, you'd need to erase std::next(I).base().

If you wanted it to be advanced by one, you'd have to do:

std::advance(I);
erase( I.base() );

The API doesn't give you enough control to make this happen right now.
One option is to make eraseFromParent return the iterators that erase
returns, and go from there.
Another is to  just add the instructions you want to erase to a
vector, and erase them later.


On Tue, Mar 31, 2015 at 6:47 PM, Mark Schimmel
<Mark.Schimmel at synopsys.com> wrote:
>   for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I != E;
> ) {
>
>
>
>     Instruction& inst = *I;
>
>
>
>     ++I; ß iterator should be advanced to the previous instruction
>
>
>
>     // Happens to be an Instruction::SExt.
>
>     // Works fine if I iterate forwards
>
>     if (isInstructionTriviallyDead(&inst, TLI))
>
>       inst.eraseFromParent();
>
>   }
>
>
>
> Sorry for the inexperienced question, but I’m confused why this works when
> using a forward iterator, but fails in reverse. The iterator is moved off
> the current instruction so why would erasing the current instruction cause
> an error?
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

Note:
This should now be fixable in trunk.

I committed a patch yesterday to make eraseFromParent return the
iterator for instructions (basic blocks coming next)

So
Instead of ++I, the loop should be:

  for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I
!= E;  ++I) {



    Instruction& inst = *I;


    if (isInstructionTriviallyDead(&inst, TLI))

      I = inst.eraseFromParent();

  }

This should work, if everything isn't lying :)



On Tue, Mar 31, 2015 at 7:08 PM, Daniel Berlin <dberlin at dberlin.org>
wrote:
> ilist uses std::reverse_iterator.
>
> reverse iterator erase  is not that simple, because
> std::reverse_iterator does not return  element, but element-1, when it
> is dereferenced.
> (IE it lies).
>
> In particular, the standard says the relationship between
> std::reverse_iterator and the base iterator you hand it is:
>
> &*(reverse_iterator(i)) == &*(base - 1)
>
>
> If you wanted it to be unchanged, you'd need to erase
std::next(I).base().
>
> If you wanted it to be advanced by one, you'd have to do:
>
> std::advance(I);
> erase( I.base() );
>
> The API doesn't give you enough control to make this happen right now.
> One option is to make eraseFromParent return the iterators that erase
> returns, and go from there.
> Another is to  just add the instructions you want to erase to a
> vector, and erase them later.
>
>
> On Tue, Mar 31, 2015 at 6:47 PM, Mark Schimmel
> <Mark.Schimmel at synopsys.com> wrote:
>>   for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I
!= E;
>> ) {
>>
>>
>>
>>     Instruction& inst = *I;
>>
>>
>>
>>     ++I; ß iterator should be advanced to the previous instruction
>>
>>
>>
>>     // Happens to be an Instruction::SExt.
>>
>>     // Works fine if I iterate forwards
>>
>>     if (isInstructionTriviallyDead(&inst, TLI))
>>
>>       inst.eraseFromParent();
>>
>>   }
>>
>>
>>
>> Sorry for the inexperienced question, but I’m confused why this works
when
>> using a forward iterator, but fails in reverse. The iterator is moved
off
>> the current instruction so why would erasing the current instruction
cause
>> an error?
>>
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>