On 7/20/21 2:08 PM, Jason Merrill wrote: > 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 checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo); >>>> +  vec 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 lower_bounds = LOOP_VINFO_LOWER_BOUNDS >> (loop_vinfo); >> +  vec 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_pair_hash; >>    hash_set compared_objects; >> >> -  vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo); >> +  vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS >> (loop_vinfo).to_vec (); > > These could also be references. Even const references it turns out for some of them. > 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. Done in the attached patch. Martin