On Mon, 7 Oct 2024, Jason Merrill wrote:
> On 10/7/24 10:26 AM, Patrick Palka wrote:
> > On Mon, 7 Oct 2024, Jason Merrill wrote:
> >
> > > On 10/7/24 9:58 AM, Patrick Palka wrote:
> > > > On Sat, 5 Oct 2024, Jason Merrill wrote:
> > > >
> > > > > On 10/4/24 11:00 AM, Patrick Palka wrote:
> > > > > > On Thu, 3 Oct 2024, Jason Merrill wrote:
> > > > > >
> > > > > > > On 10/3/24 12:38 PM, Jason Merrill wrote:
> > > > > > > > On 10/2/24 7:50 AM, Richard Biener wrote:
> > > > > > > > > This reduces peak memory usage by 20% for a specific testcase.
> > > > > > > > >
> > > > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > > > > > > > >
> > > > > > > > > It's very ugly so I'd appreciate suggestions on how to handle
> > > > > > > > > such
> > > > > > > > > situations better?
> > > > > > > >
> > > > > > > > I'm pushing this alternative patch, tested x86_64-pc-linux-gnu.
> > > > > > >
> > > > > > > OK, apparently that was both too clever and not clever enough.
> > > > > > > Replacing
> > > > > > > it
> > > > > > > with this one that's much closer to yours.
> > > > > > >
> > > > > > > Jason
> > > > > >
> > > > > > > From: Jason Merrill <ja...@redhat.com>
> > > > > > > Date: Thu, 3 Oct 2024 16:31:00 -0400
> > > > > > > Subject: [PATCH] c++: free garbage vec in coerce_template_parms
> > > > > > > To: gcc-patches@gcc.gnu.org
> > > > > > >
> > > > > > > coerce_template_parms can create two different vecs for the inner
> > > > > > > template
> > > > > > > arguments, new_inner_args and (potentially) the result of
> > > > > > > expand_template_argument_pack. One or the other, or possibly
> > > > > > > both,
> > > > > > > end up
> > > > > > > being garbage: in the typical case, the expanded vec is garbage
> > > > > > > because
> > > > > > > it's
> > > > > > > only used as the source for convert_template_argument. In some
> > > > > > > dependent
> > > > > > > cases, the new vec is garbage because we decide to return the
> > > > > > > original
> > > > > > > args
> > > > > > > instead. In these cases, ggc_free the garbage vec to reduce the
> > > > > > > memory
> > > > > > > overhead of overload resolution.
> > > > > > >
> > > > > > > gcc/cp/ChangeLog:
> > > > > > >
> > > > > > > * pt.cc (coerce_template_parms): Free garbage vecs.
> > > > > > >
> > > > > > > Co-authored-by: Richard Biener <rguent...@suse.de>
> > > > > > > ---
> > > > > > > gcc/cp/pt.cc | 10 +++++++++-
> > > > > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > > > > > > index 20affcd65a2..4ceae1d38de 100644
> > > > > > > --- a/gcc/cp/pt.cc
> > > > > > > +++ b/gcc/cp/pt.cc
> > > > > > > @@ -9275,6 +9275,7 @@ coerce_template_parms (tree parms,
> > > > > > > {
> > > > > > > /* We don't know how many args we have yet, just
> > > > > > > use the
> > > > > > > unconverted (and still packed) ones for now.
> > > > > > > */
> > > > > > > + ggc_free (new_inner_args);
> > > > > > > new_inner_args = orig_inner_args;
> > > > > > > arg_idx = nargs;
> > > > > > > break;
> > > > > > > @@ -9329,7 +9330,8 @@ coerce_template_parms (tree parms,
> > > > > > > = make_pack_expansion (conv, complain);
> > > > > > > /* We don't know how many args we have yet,
> > > > > > > just
> > > > > > > - use the unconverted ones for now. */
> > > > > > > + use the unconverted (but unpacked) ones for now. */
> > > > > > > + ggc_free (new_inner_args);
> > > > > >
> > > > > > I'm a bit worried about these ggc_frees. If an earlier template
> > > > > > parameter is a constrained auto NTTP then new_inner_args/new_args
> > > > > > could
> > > > > > have been captured by the satisfaction cache during coercion for
> > > > > > that
> > > > > > argument, and so we'd be freeing a vector that's still live?
> > > > >
> > > > > It seems like for e.g.
> > > > >
> > > > > template <class T> concept NotInt = !__is_same (T, int);
> > > > > template <NotInt auto NI, class U> struct A { };
> > > > > template <class...Ts> using B = A<'x', Ts...>;
> > > > >
> > > > > we don't check satisfaction until after we're done coercing, because
> > > > > of
> > > > >
> > > > > if (processing_template_decl && context == adc_unify)
> > > > > /* Constraints will be checked after deduction. */;
> > > > >
> > > > > in do_auto_deduction.
> > > >
> > > > Ah, I wonder why we pass/use adc_unify from both unify and
> > > > convert_template_argument.. That early exit makes sense for unify
> > > > but not for convert_template_argument since it prevents us from
> > > > checking constrained auto NTTPs during ahead of time coercion:
> > > >
> > > > template<class T>
> > > > concept C = T::value;
> > > >
> > > > template<C auto>
> > > > struct A { };
> > > >
> > > > template<class T>
> > > > void f() {
> > > > A<0> a; // no constraint error
> > > > }
> > > >
> > > > A<0> a; // constraint error
> > > >
> > > > I guess we'd ideally want to fix/implement this? At which point the
> > > > ggc_free's of new_inner_args would be unsafe I think..
> > >
> > > I don't think that would be an improvement; the C constraint is an
> > > associated
> > > constraint of A (https://eel.is/c++draft/temp#constr.decl-3.3.1).
> >
> > I thought C in 'template<C X>' is an associated constraint but not in
> > 'template<C auto X>'? At least that's how GCC currently behaves.
>
> I think that's wrong: https://eel.is/c++draft/temp#param-11
Interesting, I can try fixing that at some point.
>
> > > Why don't we check satisfaction of A<0> when parsing f?
> >
> > We do but it's considered to have no associated constraints since C is
> > 'attached' to the auto rather than to A currently.
>
> Ah, yes, I think I've run into trouble from that before.
>
> Jason
>
>
