Hi Jason, Do you have any more feedback for this patch?
Thanks, Eugene -----Original Message----- From: Eugene Rozenfeld Sent: Thursday, September 08, 2022 5:46 PM To: Jason Merrill <ja...@redhat.com>; gcc-patches@gcc.gnu.org Cc: Andi Kleen <a...@linux.intel.com>; Jan Hubicka <hubi...@ucw.cz> Subject: RE: [EXTERNAL] Re: [PING][PATCH] Add instruction level discriminator support. Jason, Thank for your suggestion. The patch is updated (attached). Thanks, Eugene -----Original Message----- From: Jason Merrill <ja...@redhat.com> Sent: Thursday, September 08, 2022 10:26 AM To: Eugene Rozenfeld <eugene.rozenf...@microsoft.com>; gcc-patches@gcc.gnu.org Cc: Andi Kleen <a...@linux.intel.com>; Jan Hubicka <hubi...@ucw.cz> Subject: Re: [EXTERNAL] Re: [PING][PATCH] Add instruction level discriminator support. On 9/1/22 18:22, Eugene Rozenfeld wrote: > Jason, > > I made another small change in addressing your feedback (attached). > > Thanks, > > Eugene > > -----Original Message----- > From: Gcc-patches > <gcc-patches-bounces+erozen=microsoft....@gcc.gnu.org> On Behalf Of > Eugene Rozenfeld via Gcc-patches > Sent: Thursday, September 01, 2022 1:49 PM > To: Jason Merrill <ja...@redhat.com>; gcc-patches@gcc.gnu.org > Cc: Andi Kleen <a...@linux.intel.com>; Jan Hubicka <hubi...@ucw.cz> > Subject: RE: [EXTERNAL] Re: [PING][PATCH] Add instruction level discriminator > support. > > Jason, > > Thank you for your review. You are correct, we only need to check > has_discriminator for the statement that's on the same line. > I updated the patch (attached). > > Thanks, > > Eugene > > -----Original Message----- > From: Jason Merrill <ja...@redhat.com> > Sent: Thursday, August 18, 2022 6:55 PM > To: Eugene Rozenfeld <eugene.rozenf...@microsoft.com>; > gcc-patches@gcc.gnu.org > Cc: Andi Kleen <a...@linux.intel.com>; Jan Hubicka <hubi...@ucw.cz> > Subject: [EXTERNAL] Re: [PING][PATCH] Add instruction level discriminator > support. > > On 8/3/22 17:25, Eugene Rozenfeld wrote: >> One more ping for this patch >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc. >> gnu.org%2Fpipermail%2Fgcc-patches%2F2022-June%2F596065.html&data= >> 0 >> 5%7C01%7Ceugene.rozenfeld%40microsoft.com%7C3e9ebe6dd5b14fe4471808da8 >> 1 >> 85dc68%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63796470932569195 >> 1 >> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6 >> I >> k1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=K%2BMx6jelnED3n%2Be2d >> T >> DYAPOqZZ8Zlsd2%2FyPJ0qib5%2FM%3D&reserved=0 >> >> CC Jason since this changes discriminators emitted in dwarf. >> >> Thanks, >> >> Eugene >> >> -----Original Message----- >> From: Eugene Rozenfeld >> Sent: Monday, June 27, 2022 12:45 PM >> To: gcc-patches@gcc.gnu.org; Andi Kleen <a...@linux.intel.com>; Jan >> Hubicka <hubi...@ucw.cz> >> Subject: RE: [PING][PATCH] Add instruction level discriminator support. >> >> Another ping for >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fpipermail%2Fgcc-patches%2F2022-June%2F596065.html&data=05%7C01%7Ceugene.rozenfeld%40microsoft.com%7Cc7c9fab519184eee0bb808da91bf2fea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637982547579085499%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Kj3YWJrDqjX%2B0Ml3At5P8NRWc1Xs6mbI%2F1vCk9%2BLaQs%3D&reserved=0 >> . >> >> I got a review from Andi >> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fpipermail%2Fgcc-patches%2F2022-June%2F596549.html&data=05%7C01%7Ceugene.rozenfeld%40microsoft.com%7Cc7c9fab519184eee0bb808da91bf2fea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637982547579085499%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NBzFtD0mH7EYKxsVWfqZgSpQmUG18SIt01XRYUlwwW4%3D&reserved=0) >> but I also need a review from someone who can approve the changes. >> >> Thanks, >> >> Eugene >> >> -----Original Message----- >> From: Eugene Rozenfeld >> Sent: Friday, June 10, 2022 12:03 PM >> To: gcc-patches@gcc.gnu.org; Andi Kleen <a...@linux.intel.com>; Jan >> Hubicka <hubi...@ucw.cz> >> Subject: [PING][PATCH] Add instruction level discriminator support. >> >> Hello, >> >> I'd like to ping this patch: >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc. >> gnu.org%2Fpipermail%2Fgcc-patches%2F2022-June%2F596065.html&data= >> 0 >> 5%7C01%7Ceugene.rozenfeld%40microsoft.com%7C3e9ebe6dd5b14fe4471808da8 >> 1 >> 85dc68%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63796470932569195 >> 1 >> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6 >> I >> k1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=K%2BMx6jelnED3n%2Be2d >> T >> DYAPOqZZ8Zlsd2%2FyPJ0qib5%2FM%3D&reserved=0 >> >> Thanks, >> >> Eugene >> >> -----Original Message----- >> From: Gcc-patches >> <gcc-patches-bounces+erozen=microsoft....@gcc.gnu.org> On Behalf Of >> Eugene Rozenfeld via Gcc-patches >> Sent: Thursday, June 02, 2022 12:22 AM >> To: gcc-patches@gcc.gnu.org; Andi Kleen <a...@linux.intel.com>; Jan >> Hubicka <hubi...@ucw.cz> >> Subject: [EXTERNAL] [PATCH] Add instruction level discriminator support. >> >> This is the first in a series of patches to enable discriminator support in >> AutoFDO. >> >> This patch switches to tracking discriminators per statement/instruction >> instead of per basic block. Tracking per basic block was problematic since >> not all statements in a basic block needed a discriminator and, also, later >> optimizations could move statements between basic blocks making correlation >> during AutoFDO compilation unreliable. Tracking per statement also allows us >> to assign different discriminators to multiple function calls in the same >> basic block. A subsequent patch will add that support. >> >> The idea of this patch is based on commit >> 4c311d95cf6d9519c3c20f641cc77af7df491fdf >> by Dehao Chen in vendors/google/heads/gcc-4_8 but uses a slightly different >> approach. In Dehao's work special (normally unused) location ids and side >> tables were used to keep track of locations with discriminators. Things have >> changed since then and I don't think we have unused location ids anymore. >> Instead, I made discriminators a part of ad-hoc locations. >> >> The difference from Dehao's work also includes support for discriminator >> reading/writing in lto streaming and in modules. >> >> Tested on x86_64-pc-linux-gnu. > >> @@ -1190,12 +1217,12 @@ assign_discriminators (void) >> || (last && same_line_p (locus, &locus_e, >> gimple_location (last)))) >> { >> - if (e->dest->discriminator != 0 && bb->discriminator == 0) >> - bb->discriminator >> - = next_discriminator_for_locus (locus_e.line); >> + if (((first && has_discriminator (gimple_location (first))) >> + || (last && has_discriminator (gimple_location (last)))) > > I think you want to check has_discriminator only for the one of first or last > that we find to have the same line as locus above? > > Incidentally, I wonder why we ignore column number here, but that's not an > issue for this patch. > > Jason This seems like excessive redundancy, especially with the slightly different names with slightly different semantics: > +++ b/gcc/input.h > +#define LOCATION_DISCRIMINATOR(LOC) \ > + ((IS_ADHOC_LOC (LOC)) ? get_discriminator_from_adhoc_loc (line_table, > (LOC)) \ > + : 0) > +extern int get_discriminator_from_locus (location_t); > +++ b/libcpp/include/line-map.h > +extern unsigned get_discriminator_from_loc (line_maps *set, > +location_t loc); Maybe gcc/input.h could overload get_discriminator_from_loc to take a single argument and forward to the libcpp version with 'line_table', and then remove both the _locus version and the macro? Jason
0001-Add-instruction-level-discriminator-support.patch
Description: 0001-Add-instruction-level-discriminator-support.patch
