> -----Original Message----- > From: Jakub Jelinek <ja...@redhat.com> > Sent: Monday, October 2, 2023 2:21 PM > To: Tamar Christina <tamar.christ...@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; jwak...@redhat.com > Subject: Re: [PATCH]middle-end: Recursively check > is_trivially_copyable_or_pair in vec.h > > On Mon, Oct 02, 2023 at 01:38:53PM +0100, Tamar Christina wrote: > > Hi All, > > > > I recently committed a patch that uses a nested std::pair in the second > argument. > > It temporarily adds a second ranking variable for sorting and then later > > drops > it. > > > > This hits the newly added assert in vec.h. This assert made some > > relaxation for std::pair but doesn't allow this case through. The > > patch allows a recursive std::pair in the second argument which fixes > bootstrap. > > I must say I still don't understand why using a struct ifcvt_arg_entry { tree > arg; > unsigned len, occur; }; with comments describing what the members mean > wouldn't be a better fix, in the sorting function what exactly means > x{1,2}.second.first and x{1,2}.second.second isn't easily understandable, > neither from the identifiers nor from any comments. > Seems because you use 2 separate vectors, one with just tree elements and > another with those tree elements + 2 unsigned values cached from it for the > sorting purpose and then rewrite the original tree vector after sorting, I > don't > really see why nested std::pair would be a better match for it than a named > structure. Furthermore, why populate args first, then compute the extra 2 > integers in another loop pushing to argsKV and finally overwrite args with > sorted values? Can't the first loop push tree with the 2 integers already? > And > what is the point of not using this structure later on when both args and > argsKV vectors are live until the end of the same function? > Can't you either pass that argsKV to others, having just one vector, or at > least > release the other vector when you don't really need it? > Formatting style, swap? arg1 : arg0 isn't correctly formatted, missing space > before ?. > > Also, ArgEntry is CamelCase which we (usually) don't use in GCC and > additionally doesn't seem to be unique enough for ODR purposes. > Ditto argsKV.
Ok, since these will take a lot longer to test and do I've reverted the patch for now since bootstrap was broken. Thanks, Tamar > > > It should also still maintain the invariant that was being tested here > > since the nested arguments should still be trivially copyable. > > > > Bootstrapped on aarch64-none-linux-gnu, x86_64-linux-gnu, and no issues. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > vec.h (struct is_trivially_copyable_or_pair): Check recursively in > > Missing "* " above. > > > second arg. > > Jakub