[llvm] r214135 - IR: Optimize size of use-list order shuffle vectors

[Duncan P. N. Exon Smith]

> On 2014-Jul-29, at 09:40, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> 
> On Tue, Jul 29, 2014 at 9:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
> Generally I'd suggest skipping emplace_back where push_back will have
> the same performance characteristics (since you don't need to call
> anything other than the move ctor, which push_back can do just fine)
> 
> But given the use of the small-size optimization, pushing back the
> result isn't ideal either, it's probably better to allocate the
> element in the Stack then initialize it there so you don't have to
> copy the small portion over:
> 
>   // Is there an easy way to make a sequence one larger and return a
> reference to that new element? It seems such a common operation...
>   Stack.resize(Stack.size() + 1);
>   UseListOrder &O = Stack.back();
>   ...
> 
> In addition/alternatively, I'm not sure how valuable the small-size
> optimization is if all of them are going in another container anyway.
> Usually the small optimization is most valuable when it avoids malloc
> allocation entirely (by using stack space) but here each element in
> Stack will have the small footprint in use - so wasted small buffers
> will be costing heap space...
> 
> (Chandler, again - thoughts on this tradeoff? I'm perhaps too readily
> open to avoiding premature optimization and thus avoid small-size
> optimizations until there's a really good reason to add them, but I
> haven't done as much profiling/performance work to have an intuition
> about when they're not premature and just good practice)
> 
> I disagree with the entire patch.
> 
> 1) We should do this unless its showing up in a profile.
> 2) We shouldn't be rolling our own datastructure here even then. We already have a TinyPtrVector which might be more appropriate *if* this is a critical memory usage problem.
> 

TinyPtrVector (which I hadn't seen, thanks for pointing it out) is
completely inappropriate.  There will be *no* vectors in here that are
size 0 or 1, the cases that TinyPtrVector optimizes for.  They'll all
be at least size 2.

I'm not too concerned.  IMO, this is simple and obviously less memory
than either std::vector<> or SmallVector.  I'm surprised there isn't
a generalized version of this available; if there had been I wouldn't
have rolled a custom one.

> I tend to agree about the small-size optimization being of dubious use when the container is being moved. I suspect std::vector is the correct datastructure until a profile shows otherwise. If it ever does, the better alternative to this might be to use pooled allocation and array refs.... But I'm skeptical about this being in the profile, and if it is in the profile, that worsens my feelings about the entire approach.

Given that this is all still experimental, I'm not at the stage of
profiling this.  I'd used `SmallVector` without really thinking about
it, and then when I noticed the error I replaced it with something
strictly better.  If you feel strongly enough that this needs to be
reverted immediately (well, switch to `std::vector`), let me know.

Otherwise, it's likely to change anyway, since shuffles don't need
nearly this much memory (see my way-too-long-to-read llvmdev post).