Re: [PATCH 1/2] vec: Add array_slice constructors from non-const and gc vectors

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]

On Mon, 29 Aug 2022, Martin Jambor wrote:

> Hi again,
> 
> On Mon, Aug 29 2022, Richard Biener wrote:
> > On Fri, 26 Aug 2022, Martin Jambor wrote:
> >
> >> Hi,
> >> 
> >> On Fri, Aug 26 2022, Richard Biener wrote:
> >> >> Am 26.08.2022 um 18:39 schrieb Martin Jambor <mjambor@suse.cz>:
> >> >>
> >> >> Hi,
> >> >>
> >> >> This patch adds constructors of array_slice that are required to
> >> >> create them from non-const (heap or auto) vectors or from GC vectors.
> >> >>
> >> >> The use of non-const array_slices is somewhat limited, as creating one
> >> >> from const vec<some_type> still leads to array_slice<const some_type>,
> >> >> so I eventually also only resorted to having read-only array_slices.
> >> >> But I do need the constructor from the gc vector.
> >> >>
> >> >> Bootstrapped and tested along code that actually uses it on
> >> >> x86_64-linux.  OK for trunk?
> >> >>
> >> >> Thanks,
> >> >>
> >> >> Martin
> >> >>
> >> >>
> >> >> gcc/ChangeLog:
> >> >>
> >> >> 2022-08-08  Martin Jambor  <mjambor@suse.cz>
> >> >>
> >> >>    * vec.h (array_slice): Add constructors for non-const reference to
> >> >>    heap vector and pointers to heap vectors.
> >> >> ---
> >> >> gcc/vec.h | 12 ++++++++++++
> >> >> 1 file changed, 12 insertions(+)
> >> >>
> >> >> diff --git a/gcc/vec.h b/gcc/vec.h
> >> >> index eed075addc9..b0477e1044c 100644
> >> >> --- a/gcc/vec.h
> >> >> +++ b/gcc/vec.h
> >> >> @@ -2264,6 +2264,18 @@ public:
> >> >>   array_slice (const vec<OtherT> &v)
> >> >>     : m_base (v.address ()), m_size (v.length ()) {}
> >> >>
> >> >> +  template<typename OtherT>
> >> >> +  array_slice (vec<OtherT> &v)
> >> >> +    : m_base (v.address ()), m_size (v.length ()) {}
> >> >> +
> >> >> +  template<typename OtherT>
> >> >> +  array_slice (const vec<OtherT, va_gc> *v)
> >> >> +    : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : 0) {}
> >> >> +
> >> >> +  template<typename OtherT>
> >> >> +  array_slice (vec<OtherT, va_gc> *v)
> >> >> +    : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : 0) {}
> >> >> +
> >> >
> >> > I don?t quite understand why the generic ctor doesn?t cover the GC case.  It looks more like reference vs pointer?
> >> >
> >> 
> >> If you think that this should work:
> >> 
> >>   vec<tree, va_gc> *heh = cfun->local_decls;
> >>   array_slice<tree> arr_slice (*heh);
> >> 
> >> then it does not:
> >> 
> >>   /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: error: no matching function for call to ?array_slice<tree_node*>::array_slice(vec<tree_node*, va_gc>&)?
> >>    6693 |   array_slice<tree> arr_slice (*heh);
> >>         |                                    ^
> >>   In file included from /home/mjambor/gcc/mine/src/gcc/hash-table.h:248,
> >>                    from /home/mjambor/gcc/mine/src/gcc/coretypes.h:486,
> >>                    from /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:105:
> >>   /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: candidate: ?template<class OtherT> array_slice<T>::array_slice(const vec<OtherT>&) [with T = tree_node*]?
> >>    2264 |   array_slice (const vec<OtherT> &v)
> >>         |   ^~~~~~~~~~~
> >>   /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note:   template argument deduction/substitution failed:
> >>   /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: note:   mismatched types ?va_heap? and ?va_gc?
> >>    6693 |   array_slice<tree> arr_slice (*heh);
> >>         |                                    ^
> >> 
> >>   [... I trimmed notes about all other candidates...]
> >> 
> >> Or did you mean something else?
> >
> > Hmm, so what if you change
> >
> >   template<typename OtherT>
> >   array_slice (const vec<OtherT> &v)
> >     : m_base (v.address ()), m_size (v.length ()) {}
> >
> > to
> >
> >   template<typename OtherT, typename l, typename a>
> >   array_slice (const vec<OtherT, l, a> &v)
> >     : m_base (v.address ()), m_size (v.length ()) {}
> >
> > instead?  Thus allow any allocation / placement template arg?
> >
> 
> So being fully awake helps, the issue was of course in how I tested the
> code, the above works fine and I can adapt my code to use that.
> 
> However, is it really preferable?
> 
> We often use NULL as to mean zero-length vector, which my code handled
> gracefully:
> 
> +  template<typename OtherT>
> +  array_slice (const vec<OtherT, va_gc> *v)
> +    : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : 0) {}
> 
> whereas using the generic method will mean that users constructing the
> vector will have to special case it - and I bet most will end up using
> the above sequence and the constructor from explicit pointer and size in
> all constructors from gc vectors.
> 
> So, should I really change the patch and my code?

Well, it's also inconsistent with a supposed use like

  vec<tree> *v = NULL;
  auto slice = array_slice (v);

no?  So, if we want to provide a "safe" (as in, handle NULL pointer)
CTOR, don't we want to handle non-GC allocated vectors the same way?

Btw, we have

  template<size_t N>
  array_slice (T (&array)[N]) : m_base (array), m_size (N) {}

which would suggest handling NULL isn't desired(?)

Richard.