[llvm-commits] [llvm] r167942 - /llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
Hal Finkel
hfinkel at anl.gov
Wed Nov 14 13:10:11 PST 2012
----- Original Message -----
> From: "Sean Silva" <silvas at purdue.edu>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Wednesday, November 14, 2012 1:45:16 PM
> Subject: Re: [llvm-commits] [llvm] r167942 - /llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
>
> > I did not mean to imply that this is not worth fixing. I plan on
> > fixing it soon (it is just a more invasive change).
>
> Ah, ok.
Upon further review, I think that the current output is now deterministic so long as the ordering of std::multimap is stable. This is not guaranteed by the C++03 standard (but, as used in BBVectorize, is this way in all known implementations, see n1780), but is guaranteed by the C++11 standard.
http://stackoverflow.com/questions/2643473/does-stdmultiset-guarantee-insertion-order?rq=1
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1780.html
I should probably get rid of the std::multimap use anyway, although I'm not sure what to replace it with. DenseMap<K, SmallVector<V, N>> perhaps?
Thanks again,
Hal
>
> > No (unless, for example, we know that they come from a
> > BumpPtrAllocator). In practice, however, I think it is common for
> > the address to be ordered w.r.t. to the allocation order (which is
> > likely to be deterministic).
>
> I thought this too when I was looking at some parts of TableGen, but
> it can actually prove to be false. BumpPtrAllocator does allocate
> sequentially, but only within each block (which is not *that* big,
> IIRC 4K by default; easily exceeded). It ultimately falls back on
> malloc() when it needs to allocate the blocks, and malloc() can
> return
> the blocks out of order.
>
> -- Sean Silva
>
> On Wed, Nov 14, 2012 at 2:40 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Sean Silva" <silvas at purdue.edu>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: llvm-commits at cs.uiuc.edu
> >> Sent: Wednesday, November 14, 2012 1:31:28 PM
> >> Subject: Re: [llvm-commits] [llvm] r167942 -
> >> /llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
> >>
> >> > There are some remaining iterations over std::*map containers
> >> > where
> >> > the order
> >> > might matter, but so long as the Value* for instructions in a
> >> > block
> >> > increase
> >> > with the order of the instructions in the block (or decrease)
> >> > monotonically,
> >> > then this will appear to be deterministic.
> >>
> >> This makes it sound like the other situations aren't worth fixing
> >> because of this assumption about the addresses (sorry if I
> >> misunderstood).
> >
> > I did not mean to imply that this is not worth fixing. I plan on
> > fixing it soon (it is just a more invasive change).
> >
> >> Do we have any reason to believe that the addresses will actually
> >> be
> >> ordered like this?
> >
> > No (unless, for example, we know that they come from a
> > BumpPtrAllocator). In practice, however, I think it is common for
> > the address to be ordered w.r.t. to the allocation order (which is
> > likely to be deterministic).
> >
> > -Hal
> >
> >>
> >> -- Sean Silva
> >>
> >> On Wed, Nov 14, 2012 at 1:38 PM, Hal Finkel <hfinkel at anl.gov>
> >> wrote:
> >> > Author: hfinkel
> >> > Date: Wed Nov 14 12:38:11 2012
> >> > New Revision: 167942
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=167942&view=rev
> >> > Log:
> >> > Fix the largest offender of determinism in BBVectorize
> >> >
> >> > Iterating over the children of each node in the potential
> >> > vectorization
> >> > plan must happen in a deterministic order (because it affects
> >> > which
> >> > children
> >> > are erased when two children conflict). There was no need for
> >> > this
> >> > data
> >> > structure to be a map in the first place, so replacing it with a
> >> > vector
> >> > is a small change.
> >> >
> >> > I believe that this was the last remaining instance if iterating
> >> > over the
> >> > elements of a Dense* container where the iteration order could
> >> > matter.
> >> > There are some remaining iterations over std::*map containers
> >> > where
> >> > the order
> >> > might matter, but so long as the Value* for instructions in a
> >> > block
> >> > increase
> >> > with the order of the instructions in the block (or decrease)
> >> > monotonically,
> >> > then this will appear to be deterministic.
> >> >
> >> > Modified:
> >> > llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
> >> >
> >> > Modified: llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
> >> > URL:
> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp?rev=167942&r1=167941&r2=167942&view=diff
> >> > ==============================================================================
> >> > --- llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp
> >> > (original)
> >> > +++ llvm/trunk/lib/Transforms/Vectorize/BBVectorize.cpp Wed Nov
> >> > 14
> >> > 12:38:11 2012
> >> > @@ -1485,7 +1485,7 @@
> >> > PrunedTree.insert(QTop.first);
> >> >
> >> > // Visit each child, pruning as necessary...
> >> > - DenseMap<ValuePair, size_t> BestChildren;
> >> > + std::vector<ValuePairWithDepth> BestChildren;
> >> > VPPIteratorPair QTopRange =
> >> > ConnectedPairs.equal_range(QTop.first);
> >> > for (std::multimap<ValuePair, ValuePair>::iterator K =
> >> > QTopRange.first;
> >> > K != QTopRange.second; ++K) {
> >> > @@ -1517,7 +1517,7 @@
> >> > DenseSet<ValuePair> CurrentPairs;
> >> >
> >> > bool CanAdd = true;
> >> > - for (DenseMap<ValuePair, size_t>::iterator C2
> >> > + for (std::vector<ValuePairWithDepth>::iterator C2
> >> > = BestChildren.begin(), E2 = BestChildren.end();
> >> > C2 != E2; ++C2) {
> >> > if (C2->first.first == C->first.first ||
> >> > @@ -1602,22 +1602,22 @@
> >> > // to an already-selected child. Check for this here,
> >> > and
> >> > if a
> >> > // conflict is found, then remove the
> >> > previously-selected
> >> > child
> >> > // before adding this one in its place.
> >> > - for (DenseMap<ValuePair, size_t>::iterator C2
> >> > + for (std::vector<ValuePairWithDepth>::iterator C2
> >> > = BestChildren.begin(); C2 !=
> >> > BestChildren.end();) {
> >> > if (C2->first.first == C->first.first ||
> >> > C2->first.first == C->first.second ||
> >> > C2->first.second == C->first.first ||
> >> > C2->first.second == C->first.second ||
> >> > pairsConflict(C2->first, C->first,
> >> > PairableInstUsers))
> >> > - BestChildren.erase(C2++);
> >> > + C2 = BestChildren.erase(C2);
> >> > else
> >> > ++C2;
> >> > }
> >> >
> >> > - BestChildren.insert(ValuePairWithDepth(C->first,
> >> > C->second));
> >> > + BestChildren.push_back(ValuePairWithDepth(C->first,
> >> > C->second));
> >> > }
> >> >
> >> > - for (DenseMap<ValuePair, size_t>::iterator C
> >> > + for (std::vector<ValuePairWithDepth>::iterator C
> >> > = BestChildren.begin(), E2 = BestChildren.end();
> >> > C != E2; ++C) {
> >> > size_t DepthF = getDepthFactor(C->first.first);
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
> > --
> > Hal Finkel
> > Postdoctoral Appointee
> > Leadership Computing Facility
> > Argonne National Laboratory
>
--
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list