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

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

[Martin Jambor]

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) {}
+
   iterator begin () { return m_base; }
   iterator end () { return m_base + m_size; }
 
-- 
2.37.2

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

[Richard Biener]

> 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
> 

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

[Martin Jambor]

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

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

[Richard Biener]

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

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
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

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

[Richard Biener]

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
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

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

[Martin Jambor]

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

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

[Martin Jambor]

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

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

[Richard Biener]

[-- 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.

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

[Richard Biener]

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.

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

[Martin Jambor]

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

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

[Richard Biener]

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

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.

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

[Richard Biener]

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.

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

[Richard Sandiford]

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