| Message ID | ri6lerbgdt4.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] vec: Add array_slice constructors from non-const and gc vectors | expand |
> 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? > iterator begin () { return m_base; } > iterator end () { return m_base + m_size; } > > -- > 2.37.2 >
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? Thanks, Martin
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? Richard. > Thanks, > > Martin >
Hi, 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? I tried this on Friday night too (but I was already only half awake) and it led to some very weird self-test ICEs (and so I went to bed). (I can try again but debugging such things is not quite what I wanted to spend my time on :-) Martin
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? Thanks, Martin
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.
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
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. Thanks and sorry for throwing in wrenches, Richard.
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
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) {} + iterator begin () { return m_base; } iterator end () { return m_base + m_size; }