On Wed, 1 Feb 2017, Jakub Jelinek wrote:
> On Wed, Feb 01, 2017 at 03:35:49PM +0100, Richard Biener wrote:
> > > > changed = false;
> > > > break;
> > > > }
> > > > - changed = true;
> > > > + if (! changed)
> > > > + {
> > > > + nelts = elts->copy ();
> > >
> > > Isn't the above part unnecessarily expensive, e.g. for the case
> > > where you have huge CONSTRUCTOR and recursive cp_fold changes already
> > > the very first value? Wouldn't it be better to just do:
> > > vec_safe_reserve (nelts, vec_safe_length (elts));
> > > vec_quick_grow (nelts, i);
> > > memcpy (nelts->address (), elts->address (),
> > > i * sizeof (constructor_elt));
> > > and then:
> >
> > It really depends on how many constructor elements usually fold.
> > If every next element will fold then yes, otherwise a memcpy
> > is going to be faster than individual vec_quick_push ()s with
> > cache-trashing cp_fold calls inbetween.
> >
> > But I didn't benchmark anything, I just looked at memory use
> > (for the case where nothing folds). And elts->copy () looks
> > much "cleaner" than this reserve/grow/memcpy ;)
>
> Maybe. But then it would be better not to do:
> + constructor_elt e = { p->index, op };
>
> + (*nelts)[i] = e;
>
> but just
> (*nelts)[i].value = op;
> because (*nelts)[i].index has been already copied and is the same,
> so no need to override it again.
Heh, yes - good catch. Consider the patch changed that way.
Richard.
