Hi Honza, Thanks for the review.
> On 13 Jan 2026, at 7:44 pm, Jan Hubicka <[email protected]> wrote: > > External email: Use caution opening links or attachments > > >> Hi Honza, >> >>> On 12 Dec 2025, at 9:04 am, Kugan Vivekanandarajah >>> <[email protected]> wrote >>> How about going with something like: >>> copyid_base = allocate_copyid_base (loc, base_discriminator); >>> This should have global context and allocate new copyid_base for each call >>> without any of the defines we have now. >>> This would still have issue for LTO where any copyid_base generated in >>> LTRANS will have conflict unless we generate this >>> in WPA and stream. What do you think? >> >> Here is the implementation (just the patch 1 or the infrastructure). Do you >> prefer this? >> >> Using this API, we would assign copy_id like: >> + gimple *nloop_last = last_nondebug_stmt (nloop->header); >> + location_t nloop_loc >> + = nloop_last ? gimple_location (nloop_last) : >> UNKNOWN_LOCATION; >> + if (nloop_loc != UNKNOWN_LOCATION) >> + { >> + unsigned int nloop_copyid = allocate_copyid_base (nloop_loc, 1); >> + assign_discriminators_to_loop (nloop, 0, nloop_copyid); >> + } >> >> >> I will post the series based on this if you prefer this. > This looks good. However I think we have problem with LTO streaming. If > some duplication happens at compile time and some at ltrans time, copyid > allocator will not see the compile time allocations. > > Fortunately I also do not see why copyid needs to be unique across > clones, since we have inline stacks. Is there one? That is true, This is mostly for within a function. > > I think the allocator needs to be per function only (stored in struct > function). When it it is initialized first time it should walk the > function body and look for existing maximal copyids for given location. Fixed this. > > Also the locations may come from inlined function (have their inline > stacks) so I think it should not be keyed by base_location, but by > base_location:inline stack. > > I think the API can stay same with this change, so I will look at the > remaining patches. > Honza Bootstrapped and regression tested on aarch64-linux-gnu with no new regressions. Is this OK? Thanks, Kugan >> >> Thanks, >> Kugan
0001-Autofdo-V5-Add-hierarchical-discriminator-support.patch
Description: 0001-Autofdo-V5-Add-hierarchical-discriminator-support.patch
