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 <mjam...@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 <mjam...@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.
Thanks and sorry for throwing in wrenches,
Richard.
