Hi Honxa, > On 15 Jan 2026, at 1:05 pm, Jan Hubicka <[email protected]> wrote: > > External email: Use caution opening links or attachments > > >> 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? > The change is still missing the walk of a body after initialization > which is needed for LTO and also after inlning when existing duplicities > can be brought to the function body. But I think it can be done > incrementally, so the patch is OK.
Please see the attached patch that adds walking the body and updates the map. Bootstrap and regression test on aarch64-linux-gnu is progressing. Is this OK if no regressions? Thanks, Kugan > Honza >> >> Thanks, >> Kugan >> >> >>>> >>>> Thanks, >>>> Kugan >> >> > >
0001-AutoFDO-Walk-function-body-to-to-find-existing-max-c.patch
Description: 0001-AutoFDO-Walk-function-body-to-to-find-existing-max-c.patch
