[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2056-2059
+  void replaceRawLinkageName(MDString *LinkageName) {
+    replaceOperandWith(3, LinkageName);
+  }
+
----------------
tmsriram wrote:
> dblaikie wrote:
> > The need to add this API makes me a bit uncertain that it's the right direction - any chance you could find other examples of Function duplication in LLVM passes (maybe the partial inliner? Perhaps when it partially inlines an external function it has to clone the function so the external version remains identical?) and how they deal with debug info? Perhaps there's an existing API/mechanism to update the DISubprogram as you want without adding this.
> This does not seem like a radical new API to me.  We could use existing setOperand or replaceOperandWith here but need a public wrapper to expose it, just like getRawLinkageName.  For example, DICompositeType is mutated like this with buildODRType.
Yeah, it would be nice to leverage an existing API like this but unfortunately I haven't found any.

Regarding passes that duplicate functions, I think the renaming done by `UniqueInternalLinkageNamesPass` is transparent to subsequent passes, since it is placed very early in the pipeline before any inlining or cloning passes.


================
Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:26-29
+static cl::opt<bool> UniqueDebugAndProfileNames(
+    "unqiue-debug-profile-names", cl::init(true), cl::Hidden,
+    cl::desc("Uniqueify debug and profile symbol Names"));
+
----------------
dblaikie wrote:
> Do you have plans to use the flag in testing, etc? If not, I guess you might as well bake in the behavior you want if no one else is using this/there's no current use case for the flexibility?
I'm not quite sure if updating debug names are required in all use cases. @tmsriram do you think we can just remove the flag?


================
Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:48
+      if (UniqueDebugAndProfileNames) {
+        F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+        if (DISubprogram *SP = F.getSubprogram()) {
----------------
dblaikie wrote:
> What's this about/for? Should this be in an independent change?
This is about how the sample profile loader deals with renamed functions. The sample loader will use the original function name (i.e, by ignoring all suffixes appended) to retrieve a profile by default. With the attribute set here. the sample loader will only ignore certain suffixes not including the one (.__uniq.) appended here. Please see `getCanonicalFnName` in `SampleProf.h` for details. The reason that names are uniquefied here for AutoFDO is to have the sample loader differentiate same-named static functions, so the change is sort of related here.

I'll put more comments for that.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93656/new/

https://reviews.llvm.org/D93656