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

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Mon, Aug 29 2022, Richard Biener wrote:
> 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?
>

Our safe_* functions usually do no work with normal non-GC vectors
(which are not vl_embed), they do not accept them.  I guess that is
because that is not how we use normal vectors, we usually pass around
vNULL to mean empty vector of that type.  So I'd at least be consistent
with our inconsistencies.

But whatever, I can have both reference and pointer template
constructors, I can resort to constructing them from v->address() and
v->length() too.  I do not care much, I guess I trust your sense of code
esthetics more than mine, just please let me know what you prefer and
I'll go with that.

> 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(?)
>

That is not how I read for example:

  // True if the array is valid, false if it is an array like INVALID.
  bool is_valid () const { return m_base || m_size == 0; }

And IMHO it would be a very very strange limitation too.

Martin