Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)

[Jason Merrill <jason@redhat.com>]

On 7/20/21 2:34 PM, Martin Sebor wrote:
> On 7/14/21 10:23 AM, Jason Merrill wrote:
>> On 7/14/21 10:46 AM, Martin Sebor wrote:
>>> On 7/13/21 9:39 PM, Jason Merrill wrote:
>>>> On 7/13/21 4:02 PM, Martin Sebor wrote:
>>>>> On 7/13/21 12:37 PM, Jason Merrill wrote:
>>>>>> On 7/13/21 10:08 AM, Jonathan Wakely wrote:
>>>>>>> On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
>>>>>>>> Somebody with more C++ knowledge than me needs to approve the
>>>>>>>> vec.h changes - I don't feel competent to assess all effects of 
>>>>>>>> the change.
>>>>>>>
>>>>>>> They look OK to me except for:
>>>>>>>
>>>>>>> -extern vnull vNULL;
>>>>>>> +static constexpr vnull vNULL{ };
>>>>>>>
>>>>>>> Making vNULL have static linkage can make it an ODR violation to use
>>>>>>> vNULL in templates and inline functions, because different
>>>>>>> instantiations will refer to a different "vNULL" in each translation
>>>>>>> unit.
>>>>>>
>>>>>> The ODR says this is OK because it's a literal constant with the 
>>>>>> same value (6.2/12.2.1).
>>>>>>
>>>>>> But it would be better without the explicit 'static'; then in 
>>>>>> C++17 it's implicitly inline instead of static.
>>>>>
>>>>> I'll remove the static.
>>>>>
>>>>>>
>>>>>> But then, do we really want to keep vNULL at all?  It's a weird 
>>>>>> blurring of the object/pointer boundary that is also dependent on 
>>>>>> vec being a thin wrapper around a pointer.  In almost all cases it 
>>>>>> can be replaced with {}; one exception is == comparison, where it 
>>>>>> seems to be testing that the embedded pointer is null, which is a 
>>>>>> weird thing to want to test.
>>>>>
>>>>> The one use case I know of for vNULL where I can't think of
>>>>> an equally good substitute is in passing a vec as an argument by
>>>>> value.  The only way to do that that I can think of is to name
>>>>> the full vec type (i.e., the specialization) which is more typing
>>>>> and less generic than vNULL.  I don't use vNULL myself so I wouldn't
>>>>> miss this trick if it were to be removed but others might feel
>>>>> differently.
>>>>
>>>> In C++11, it can be replaced by {} in that context as well.
>>>
>>> Cool.  I thought I'd tried { } here but I guess not.
>>>
>>>>
>>>>> If not, I'm all for getting rid of vNULL but with over 350 uses
>>>>> of it left, unless there's some clever trick to make the removal
>>>>> (mostly) effortless and seamless, I'd much rather do it independently
>>>>> of this initial change. I also don't know if I can commit to making
>>>>> all this cleanup.
>>>>
>>>> I already have a patch to replace all but one use of vNULL, but I'll 
>>>> hold off with it until after your patch.
>>>
>>> So what's the next step?  The patch only removes a few uses of vNULL
>>> but doesn't add any.  Is it good to go as is (without the static and
>>> with the additional const changes Richard suggested)?  This patch is
>>> attached to my reply to Richard:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
>>
>> As Richard wrote:
>>
>>> The pieces where you change vec<> passing to const vec<>& and the few
>>> where you change vec<> * to const vec<> * are OK - this should make the
>>> rest a smaller piece to review.
>>
>> Please go ahead and apply those changes and send a new patch with the 
>> remainder of the changes.
> 
> I have just pushed r12-2418:
> https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
> 
>>
>> A few other comments:
>>
>>> -                       omp_declare_simd_clauses);
>>> +                       *omp_declare_simd_clauses);
>>
>> Instead of doing this indirection in all of the callers, let's change 
>> c_finish_omp_declare_simd to take a pointer as well, and do the 
>> indirection in initializing a reference variable at the top of the 
>> function.
> 
> Okay.
> 
>>
>>> +    sched_init_luids (bbs.to_vec ());
>>> +    haifa_init_h_i_d (bbs.to_vec ());
>>
>> Why are these to_vec changes needed when you are also changing the 
>> functions to take const&?
> 
> Calling to_vec() here isn't necessary so I've removed it.
> 
>>
>>> -  vec<tree> checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
>>> +  vec<tree> checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
>>
>> Why not use a reference here and in other similar spots?
> 
> Sure, that works too.
> 
> Attached is what's left of the original changes now that r12-2418
> has been applied.

> @@ -3364,7 +3364,8 @@ static void
>  vect_check_lower_bound (loop_vec_info loop_vinfo, tree expr, bool unsigned_p,
>  			poly_uint64 min_value)
>  {
> -  vec<vec_lower_bound> lower_bounds = LOOP_VINFO_LOWER_BOUNDS (loop_vinfo);
> +  vec<vec_lower_bound> lower_bounds
> +    = LOOP_VINFO_LOWER_BOUNDS (loop_vinfo).to_vec ();
>    for (unsigned int i = 0; i < lower_bounds.length (); ++i)
>      if (operand_equal_p (lower_bounds[i].expr, expr, 0))
>        {
> @@ -3466,7 +3467,7 @@ vect_prune_runtime_alias_test_list (loop_vec_info loop_vinfo)
>    typedef pair_hash <tree_operand_hash, tree_operand_hash> tree_pair_hash;
>    hash_set <tree_pair_hash> compared_objects;
>  
> -  vec<ddr_p> may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo);
> +  vec<ddr_p> may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo).to_vec ();

These could also be references.

That leaves this as the only remaining use of to_vec:

>    ipa_call_arg_values (ipa_auto_call_arg_values *aavals)
> -    : m_known_vals (aavals->m_known_vals),
> -      m_known_contexts (aavals->m_known_contexts),
> -      m_known_aggs (aavals->m_known_aggs),
> -      m_known_value_ranges (aavals->m_known_value_ranges)
> +    : m_known_vals (aavals->m_known_vals.to_vec ()),
> +      m_known_contexts (aavals->m_known_contexts.to_vec ()),
> +      m_known_aggs (aavals->m_known_aggs.to_vec ()),
> +      m_known_value_ranges (aavals->m_known_value_ranges.to_vec ())

I think we could handle this by deriving ipa_auto_call_arg_values from 
ipa_call_arg_values like auto_vec is derived from vec, but perhaps 
dealing with the IPA datastructures could be saved for the next stage of 
overhauling vec.  Maybe for now just change the name to_vec to something 
clearer that new code shouldn't use it, e.g. to_vec_legacy.

Jason