david-xl wrote: > > > > > > David says the itanium remapper file was only used once during gcc > > > > > > to llvm transition, so not relevant here. > > > > > > > > > > > > > > > I believe it was actually for the libstdc++ to libc++ transition (see > > > > > https://reviews.llvm.org/D51247 and https://reviews.llvm.org/D51240). > > > > > If it is broken we'll at least want to add a FIXME there. > > > > > > > > > > > > Yes, I meant libstdc++ to libc++ transition. Why source line is this > > > > comment addressing? I take take a look the changes/comments there. > > > > > > > > > Sorry for the misinformation, and thanks for the Phab links. > > > I think the itanium remapper needs a `:` -> `;` update (going to update > > > this PR and related tests), since (for local-linkage functions) the > > > function name used to look up profiles should use `;` delimiter. > > > > > > > > > > David says the itanium remapper file was only used once during gcc > > > > > > to llvm transition, so not relevant here. > > > > > > > > > > > > > > > I believe it was actually for the libstdc++ to libc++ transition (see > > > > > https://reviews.llvm.org/D51247 and https://reviews.llvm.org/D51240). > > > > > If it is broken we'll at least want to add a FIXME there. > > > > > > > > > > > > Yes, I meant libstdc++ to libc++ transition. Why source line is this > > > > comment addressing? I take take a look the changes/comments there. > > > > > > > > > Sorry for the misinformation, and thanks for the Phab links. > > > I think the itanium remapper needs a `:` -> `;` update (going to update > > > this PR and related tests), since (for local-linkage functions) the > > > function name used to look up profiles should use `;` delimiter. > > > > > > The remapper is not aware of any internal symbol mangling scheme, so those > > entires won't be tracked by it. In other words, there is no need to change > > anything there, I think. > > Not updating Itanium remapper should work for PGO counter matching until the > next transition (details below); for consistency I just updated Itanium > remapper's `extractName` to use semicolon. > > The details > > * For PGO counter matching, instr prof reader > [asks](https://github.com/llvm/llvm-project/blob/32ec5fbfed32f37aa070ee38e9b038bd84ca6479/llvm/lib/ProfileData/InstrProfReader.cpp#L1339) > read remapper for a record. > * Only with a remapping file provided (specified by > `-fprofile-remapping-file`), a itanium remap reader is constructed. And when > remapping file is not specified, a no-op remap reader is constructed. [source > code](https://github.com/llvm/llvm-project/blob/32ec5fbfed32f37aa070ee38e9b038bd84ca6479/llvm/lib/ProfileData/InstrProfReader.cpp#L1306-L1314) > * When remapping file is specified, remap reader tries to extract the mangled > name (removing `filename` prefix`) by finding a `:`(no longer used as > delimiter for newer profiles) and remaps the mangled name. If`:`is not > updated to`;`, the name is remapped to itself (irpgo func format) and > profiles could still be found. However, not updating means remapping becomes > no-op for local-linkage functions, which is fine after the transition > complete but doesn't work for new transitions (if it happens..)
Sounds good. I checked the history of remapping for instrProfile. For IR PGO, there is basically no need to do so as the instrumentation and profile-use should be in-sync. For front-end instrumentation, there seem to be some use cases to use out of sync profile: https://reviews.llvm.org/D51240. The remapping is also best effort -- i.e. not guarantee to keep all the old profile e.g. icall targets are not supported. https://github.com/llvm/llvm-project/pull/74008 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
