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

[Richard Sandiford <richard.sandiford@arm.com>]

Richard Biener <rguenther@suse.de> writes:
> On Mon, 29 Aug 2022, Martin Jambor wrote:
>
>> 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.
>
> I see.  That said, the high number of CTORs does look a bit odd,
> but I'm fine with them if Richard is.

Yeah, the patch LGTM FWWIW.  I agree it feels a bit weird to convert
"pointer to vector of T" into "array-like of T" without a dereference,
but avoiding it might be more convoluted than going with the flow.

It doesn't look like it should introduce genuine ambiguity, since the
T template parameter would always need to be specified explicitly.
(But I don't think we should have a make_array_slice for vector pointers.)

Thanks,
Richard