[llvm-commits] [llvm] r165065 - in /llvm/trunk: include/llvm/ADT/SetVector.h lib/Transforms/Scalar/SROA.cpp test/Transforms/SROA/basictest.ll
Chandler Carruth
chandlerc at gmail.com
Tue Oct 2 17:05:50 PDT 2012
On Tue, Oct 2, 2012 at 5:05 PM, Chandler Carruth <chandlerc at gmail.com>wrote:
> On Tue, Oct 2, 2012 at 4:26 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>> > + template <typename UnaryPredicate>
>> > + bool remove_if(UnaryPredicate P) {
>> > + typename vector_type::iterator B = std::remove_if(vector_.begin(),
>> > + vector_.end(),
>> P),
>> > + E = vector_.end();
>> > + if (B == E)
>> > + return false;
>> > + for (typename vector_type::iterator I = B; I != E; ++I)
>> > + set_.erase(*I);
>> > + vector_.erase(B, E);
>> > + return true;
>> > + }
>> > +
>>
>> I don't think that remove_if is allowed to be used like this.
>>
>> From 25.3.8 [alg.remove]:
>> Note: each element in the range [ret,last), where ret is the returned
>> value, has a valid but unspeciļ¬ed state, because the algorithms can
>> eliminate elements by swapping with or moving from elements that were
>> originally in that range.
>>
>>
>> I think you can use stable_partition() to do what you want here, but
>> that's pretty expensive (comparatively; it works by recursively
>> rotating in specific ways; n*log n but also the constant factor
>> compared with just a linear scan of an array). The simplest
>> alternative is probably to just open code remove_if(), and do the
>> deletion inside the open-coded remove_if() loop as you "delete"
>> elements (i.e. overwrite them).
>>
>
> Thanks, this is a great point.
>
> No need to write out remove_if -- std::partition() should be just as
> efficient and provides the guarantee we need.
>
And for clarity, I fixed this in r165073.
>
>
>>
>> --Sean Silva
>>
>> On Tue, Oct 2, 2012 at 6:46 PM, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>> > Author: chandlerc
>> > Date: Tue Oct 2 17:46:45 2012
>> > New Revision: 165065
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=165065&view=rev
>> > Log:
>> > Teach the new SROA to handle cases where an alloca that has already been
>> > scheduled for processing on the worklist eventually gets deleted while
>> > we are processing another alloca, fixing the original test case in
>> > PR13990.
>> >
>> > To facilitate this, add a remove_if helper to the SetVector abstraction.
>> > It's not easy to use the standard abstractions for this because of the
>> > specifics of SetVectors types and implementation.
>> >
>> > Finally, a nice small test case is included. Thanks to Benjamin for the
>> > fantastic reduced test case here! All I had to do was delete some empty
>> > basic blocks!
>> >
>> > Modified:
>> > llvm/trunk/include/llvm/ADT/SetVector.h
>> > llvm/trunk/lib/Transforms/Scalar/SROA.cpp
>> > llvm/trunk/test/Transforms/SROA/basictest.ll
>> >
>> > Modified: llvm/trunk/include/llvm/ADT/SetVector.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SetVector.h?rev=165065&r1=165064&r2=165065&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/ADT/SetVector.h (original)
>> > +++ llvm/trunk/include/llvm/ADT/SetVector.h Tue Oct 2 17:46:45 2012
>> > @@ -126,6 +126,32 @@
>> > return false;
>> > }
>> >
>> > + /// \brief Remove items from the set vector based on a predicate
>> function.
>> > + ///
>> > + /// This is intended to be equivalent to the following code, if we
>> could
>> > + /// write it:
>> > + ///
>> > + /// \code
>> > + /// V.erase(std::remove_if(V.begin(), V.end(), P), V.end());
>> > + /// \endcode
>> > + ///
>> > + /// However, SetVector doesn't expose non-const iterators, making any
>> > + /// algorithm like remove_if impossible to use.
>> > + ///
>> > + /// \returns true if any element is removed.
>> > + template <typename UnaryPredicate>
>> > + bool remove_if(UnaryPredicate P) {
>> > + typename vector_type::iterator B = std::remove_if(vector_.begin(),
>> > + vector_.end(),
>> P),
>> > + E = vector_.end();
>> > + if (B == E)
>> > + return false;
>> > + for (typename vector_type::iterator I = B; I != E; ++I)
>> > + set_.erase(*I);
>> > + vector_.erase(B, E);
>> > + return true;
>> > + }
>> > +
>> >
>> > /// \brief Count the number of elements of a given key in the
>> SetVector.
>> > /// \returns 0 if the element is not in the SetVector, 1 if it is.
>> >
>> > Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=165065&r1=165064&r2=165065&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)
>> > +++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Tue Oct 2 17:46:45 2012
>> > @@ -3302,7 +3302,11 @@
>> > while (!Worklist.empty()) {
>> > Changed |= runOnAlloca(*Worklist.pop_back_val());
>> > deleteDeadInstructions(DeletedAllocas);
>> > +
>> > + // Remove the deleted allocas from various lists so that we don't
>> try to
>> > + // continue processing them.
>> > if (!DeletedAllocas.empty()) {
>> > + Worklist.remove_if(IsAllocaInSet(DeletedAllocas));
>> > PromotableAllocas.erase(std::remove_if(PromotableAllocas.begin(),
>> > PromotableAllocas.end(),
>> >
>> IsAllocaInSet(DeletedAllocas)),
>> >
>> > Modified: llvm/trunk/test/Transforms/SROA/basictest.ll
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/basictest.ll?rev=165065&r1=165064&r2=165065&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/test/Transforms/SROA/basictest.ll (original)
>> > +++ llvm/trunk/test/Transforms/SROA/basictest.ll Tue Oct 2 17:46:45
>> 2012
>> > @@ -897,3 +897,32 @@
>> > %tmp2 = load i8* %gep
>> > ret void
>> > }
>> > +
>> > +define void @PR13990() {
>> > +; Ensure we can handle cases where processing one alloca causes the
>> other
>> > +; alloca to become dead and get deleted. This might crash or fail under
>> > +; Valgrind if we regress.
>> > +; CHECK: @PR13990
>> > +; CHECK-NOT: alloca
>> > +; CHECK: unreachable
>> > +; CHECK: unreachable
>> > +
>> > +entry:
>> > + %tmp1 = alloca i8*
>> > + %tmp2 = alloca i8*
>> > + br i1 undef, label %bb1, label %bb2
>> > +
>> > +bb1:
>> > + store i8* undef, i8** %tmp2
>> > + br i1 undef, label %bb2, label %bb3
>> > +
>> > +bb2:
>> > + %tmp50 = select i1 undef, i8** %tmp2, i8** %tmp1
>> > + br i1 undef, label %bb3, label %bb4
>> > +
>> > +bb3:
>> > + unreachable
>> > +
>> > +bb4:
>> > + unreachable
>> > +}
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121002/28bb006a/attachment.html>
More information about the llvm-commits
mailing list