Hi Honza, Thanks for the review.
> On 1 Dec 2025, at 10:57 pm, Jan Hubicka <[email protected]> wrote: > > External email: Use caution opening links or attachments > > >> gcc/ChangeLog: >> >> * Makefile.in: Add hierarchical_discriminator.o to OBJS. >> * hierarchical_discriminator.cc: New file. >> * hierarchical_discriminator.h: New file. >> * input.cc (location_with_discriminator_components): New function. >> (get_discriminator_components_from_loc): Likewise. >> * input.h (DISCR_BASE_BITS): New constant. >> (DISCR_MULTIPLICITY_BITS): Likewise. >> (DISCR_COPYID_BITS): Likewise. >> (DISCR_UNUSED_BITS): Likewise. >> (DISCR_BASE_MASK): Likewise. >> (DISCR_MULTIPLICITY_MASK): Likewise. >> (DISCR_COPYID_MASK): Likewise. >> (DISCR_BASE_SHIFT): Likewise. >> (DISCR_MULTIPLICITY_SHIFT): Likewise. >> (DISCR_COPYID_SHIFT): Likewise. >> (DISCR_BASE_MAX): Likewise. >> (DISCR_MULTIPLICITY_MAX): Likewise. >> (DISCR_COPYID_MAX): Likewise. >> (location_with_discriminator_components): New function declaration. >> (get_discriminator_components_from_loc): Likewise. >> >> Signed-off-by: Kugan Vivekanandarajah <[email protected]> >> >> Thanks, >> Kugan >> >> >> From 46968fce3960772ddbefb9e62bde7512fd8fb8b3 Mon Sep 17 00:00:00 2001 >> From: Kugan Vivekanandarajah <[email protected]> >> Date: Tue, 11 Nov 2025 18:00:08 -0800 >> Subject: [PATCH 1/4] [Autofdo] Add hierarchical discriminator support >> >> This patch introduces the basic infrastructure for hierarchical >> discriminators with format [Base:8][Multiplicity:7][CopyID:11][Unused:6]. >> It adds helper functions to create and extract discriminator components. >> >> gcc/ChangeLog: >> >> * Makefile.in: Add hierarchical_discriminator.o to OBJS. >> * hierarchical_discriminator.cc: New file. >> * hierarchical_discriminator.h: New file. >> * input.cc (location_with_discriminator_components): New function. >> (get_discriminator_components_from_loc): Likewise. >> * input.h (DISCR_BASE_BITS): New constant. >> (DISCR_MULTIPLICITY_BITS): Likewise. >> (DISCR_COPYID_BITS): Likewise. >> (DISCR_UNUSED_BITS): Likewise. >> (DISCR_BASE_MASK): Likewise. >> (DISCR_MULTIPLICITY_MASK): Likewise. >> (DISCR_COPYID_MASK): Likewise. >> (DISCR_BASE_SHIFT): Likewise. >> (DISCR_MULTIPLICITY_SHIFT): Likewise. >> (DISCR_COPYID_SHIFT): Likewise. >> (DISCR_BASE_MAX): Likewise. >> (DISCR_MULTIPLICITY_MAX): Likewise. >> (DISCR_COPYID_MAX): Likewise. >> (location_with_discriminator_components): New function declaration. >> (get_discriminator_components_from_loc): Likewise. >> >> Signed-off-by: Kugan Vivekanandarajah <[email protected]> >> --- >> gcc/Makefile.in | 1 + >> gcc/hierarchical_discriminator.cc | 108 ++++++++++++++++++++++++++++++ >> gcc/hierarchical_discriminator.h | 80 ++++++++++++++++++++++ >> gcc/input.cc | 35 ++++++++++ >> gcc/input.h | 40 +++++++++++ >> 5 files changed, 264 insertions(+) >> create mode 100644 gcc/hierarchical_discriminator.cc >> create mode 100644 gcc/hierarchical_discriminator.h >> > >> +/* Assign discriminators to all statements in a basic block. This >> + function updates the multiplicity and/or copyid discriminator components >> for >> + all statements in the given basic block, while preserving the base >> + discriminator. */ >> + >> +void >> +assign_discriminators_to_bb (basic_block bb, >> + unsigned int multiplicity_value, >> + unsigned int copyid_value, >> + bool update_multiplicity, >> + bool update_copyid) > > I would also add stmt version.... How do we want to use the version? >> +{ >> + gimple_stmt_iterator gsi; >> + >> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >> + { >> + gimple *stmt = gsi_stmt (gsi); >> + location_t loc = gimple_location (stmt); >> + >> + if (loc == UNKNOWN_LOCATION || is_gimple_debug (stmt)) >> + continue; >> + >> + /* Get existing discriminator components. */ >> + unsigned int base, multiplicity, copyid; >> + get_discriminator_components_from_loc (loc, &base, >> + &multiplicity, ©id); >> + >> + /* Update requested components. */ >> + if (update_multiplicity) >> + multiplicity = multiplicity_value; > > I wonder if it should not be multiplicity *=, since transformations can > combine. >> + if (update_copyid) >> + copyid = copyid_value; >> + >> + /* Set new location. */ >> + location_t new_loc = location_with_discriminator_components (loc, >> base, >> + >> multiplicity, >> + copyid); >> + gimple_set_location (stmt, new_loc); > ... > the reason is that there are some extra locators to update. PHI > statmenets, goto edges and also locations which can be placed in stmt > operands. >> + >> +/* Hierarchical discriminator layout (32 bits total): >> + Discriminator format: [Base:8][Multiplicity:7][CopyID:11][Unused:6] >> + - Base: bits 0-7 (8 bits, 0-255) >> + - Multiplicity: bits 8-14 (7 bits, 0-127) >> + - CopyID: bits 15-25 (11 bits, 0-2047) >> + - Unused: bits 26-31 (6 bits, reserved) >> + >> + Base discriminator: Used by front-end and early passes to distinguish >> + different statements on the same source line. >> + >> + Multiplicity: Duplication factor for unrolling/vectorization. >> + Represents how many times code is duplicated: >> + - Loop unrolling factor >> + - Vectorization width >> + >> + CopyID: Unique identifier for code copies to distinguish: >> + - Inlining contexts (different callsites) >> + - Function cloning >> + >> + Note: Loop unrolling and versioning use multiplicity, not copyid. >> + */ >> + >> +/* Loop versioning discriminators (CopyID values). */ >> +#define DISCRIMINATOR_LOOP_VERSION_VECTORIZED 1 /* Vectorized version. */ >> +#define DISCRIMINATOR_LOOP_VERSION_SCALAR 2 /* Scalar version. */ >> +#define DISCRIMINATOR_LOOP_VERSION_ALIGNED 3 /* Aligned version. */ >> +#define DISCRIMINATOR_LOOP_VERSION_UNALIGNED 4 /* Unaligned version. */ >> +#define DISCRIMINATOR_LOOP_PROLOG 6 /* Prolog loop. */ >> +#define DISCRIMINATOR_LOOP_EPILOG 7 /* Epilog loop. */ >> +#define DISCRIMINATOR_LOOP_EPILOG_VECTORIZED 8 /* Vector epilog loop. */ > > It is still not quite clear to me we want to go with pre-assigned copy > ID numbers. Optimizations can happen mulitple times at same loop (for > exmaple cascaded prologs). But I guess this is a good start, since it > is simple. I was also thinking about this. Not sure what w would be a best way to tackle instructions evolving two diffent paths. We want to make sure we dont end with the same copy_id. >> + >> +/* Helper function to assign discriminators to all statements in a basic >> + block. This preserves the base discriminator and only updates the >> + requested components. */ >> +extern void assign_discriminators_to_bb (basic_block bb, >> + unsigned int multiplicity_value, >> + unsigned int copyid_value, >> + bool update_multiplicity, >> + bool update_copyid); >> + >> +/* Helper function to assign discriminators to all basic blocks in a loop. >> + This is used by loop versioning passes to distinguish different versions >> + of the same loop and to indicate vectorization factors. >> + >> + multiplicity_value: vectorization factor (0 to keep unchanged) >> + copyid_value: version ID (1=vectorized, 2=scalar, etc.) */ >> +extern void assign_discriminators_to_loop (class loop *loop, >> + unsigned int multiplicity_value, >> + unsigned int copyid_value); >> + >> +#endif /* GCC_HIERARCHICAL_DISCRIMINATOR_H. */ >> diff --git a/gcc/input.cc b/gcc/input.cc >> index aad98394711..520ae33add8 100644 >> --- a/gcc/input.cc >> +++ b/gcc/input.cc >> @@ -1074,6 +1074,41 @@ get_discriminator_from_loc (location_t locus) >> return get_discriminator_from_loc (line_table, locus); >> } >> >> +/* Create a location with hierarchical discriminator components. */ >> + >> +location_t >> +location_with_discriminator_components (location_t locus, >> + unsigned int base, >> + unsigned int multiplicity, >> + unsigned int copyid) > > Can you please put these values into a structure? > I think it will be then easier to add possible additional fields > and will be more handy to work with. > > The patch looks good otherwise, but I think Jakub or Jason > (with more dwarf knowledge than me) probably should have chance to > comment. Since LLVM uses same scheme, I hope there is no problem with > having much bigger discriminators than before. Here is a revised patch. Is this OK? Thanks Kugan > > Honza
0001-Autofdo-V2-Add-hierarchical-discriminator-support.patch
Description: 0001-Autofdo-V2-Add-hierarchical-discriminator-support.patch
