On Wed, Feb 01, 2017 at 03:03:15PM +0100, Richard Biener wrote:
> 2017-02-01 Richard Biener <[email protected]>
>
> PR cp/14179
> cp/
> * cp-gimplify.c (cp_fold): When folding a CONSTRUCTOR copy
> it lazily on the first changed element only and copy it
> fully upfront, only storing changed elements.
>
> Index: gcc/cp/cp-gimplify.c
> ===================================================================
> --- gcc/cp/cp-gimplify.c (revision 245094)
> +++ gcc/cp/cp-gimplify.c (working copy)
> @@ -2361,12 +2361,9 @@ cp_fold (tree x)
> bool changed = false;
> vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (x);
> vec<constructor_elt, va_gc> *nelts = NULL;
> - vec_safe_reserve (nelts, vec_safe_length (elts));
> FOR_EACH_VEC_SAFE_ELT (elts, i, p)
> {
> tree op = cp_fold (p->value);
> - constructor_elt e = { p->index, op };
> - nelts->quick_push (e);
> if (op != p->value)
> {
> if (op == error_mark_node)
> @@ -2375,7 +2372,13 @@ cp_fold (tree x)
> 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:
> + changed = true;
> + }
> + constructor_elt e = { p->index, op };
> + (*nelts)[i] = e;
Remove the above two lines:
> }
and add:
if (changed)
{
constructor_elt e = { p->index, op };
nelts->quick_push (e);
}
> }
> if (changed)
Otherwise it looks good to me, but Jason should have his last word.
Jakub
