[PATCH] D51248: Support for remapping profile data, for sample-based profiling.

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

In https://reviews.llvm.org/D51248#1214761, @wmi wrote:

> > We do not support remapping indirect branch target information, but all other profile data should be remapped appropriately.
>
> Any particular reason for that? Without support of call target, the performance could still be hurt badly after the renaming.


Yes, the idea here is that the user doesn't need to specify which mappings have already happened (there is no "before" / "after", and no "remangling" step, just two name components that are to be treated as "the same"). That works well for most profile data: we can intercept queries for a call to `foo(std::__1::string)` and look up any names that are "similar" to that. But it doesn't work so easily for indirect call targets, where we would need to check (demangle and attempt to match) all functions within the module in order to determine what a particular indirect call target is transformed into. If it's important to be able to handle this, I think the cleanest way to do it would be for the profile data consumer to pass a list of all "known" function names into the reader to support such remapping. (Another approach would be to add a remangling step and require the equivalences to be specified as before -> after transformations, but that would add substantially more complexity and puts a maintenance burden on all future extensions to the demangler, so it wouldn't be my preference.)

I think this is something that we should incrementally add later, even if we already know for sure that we'll need it.



================
Comment at: include/llvm/ProfileData/SampleProfReader.h:518
 
+class SampleProfileReaderItaniumRemapper : public SampleProfileReader {
+public:
----------------
davidxl wrote:
> Can add some documentation on the class.
> 
> Also why put Itanium in the class name? The user provided remapping file should be independent of any C++ ABIs.
(Comment added.)

Remapping is supported only for Itanium manglings for now, and that's all this class supports as a consequence. While I think it should be feasible to support remapping MS ABI manglings with largely the same interface, it seems reasonable to me to give this class the more specific name until we generalize it.

But I don't feel at all strongly about this, and I'm happy change the name if you prefer. If we drop the `Itanium` here, I think it'd make sense to also drop it from the `ProfileRemappingReader` with a view to it (eventually) hiding the difference between the mangling schemes there.


Repository:
  rL LLVM

https://reviews.llvm.org/D51248