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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: rnk.
dblaikie added a comment.

This should have a LLVM test coverage for the LLVM changes. (I realize they're also tested by the Clang test, because there's no way to test Clang's pass manager creation short of testing the effect of running the pass manager (hmm - /maybe/ there's a way to dump the pass pipeline? In which case that's how Clang should be tested, just testing that it creates the right pipeline, not that that pipeline does any particular thing))

In D93656#2466689 <https://reviews.llvm.org/D93656#2466689>, @tmsriram wrote:

> https://reviews.llvm.org/D73307#1932131  rnk@ mentioned this :  "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."
>
> I just wanted to mention it  here that this was anticipated and was missed in the original patch, my bad as I didnt think about DebugInfo change.  However, I think it is pretty straightforward to change the linkage name so I would still keep the pass approach.

@rnk - any thoughts on this?



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2056-2059
+  void replaceRawLinkageName(MDString *LinkageName) {
+    replaceOperandWith(3, LinkageName);
+  }
+
----------------
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.


================
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"));
+
----------------
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?


================
Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:48
+      if (UniqueDebugAndProfileNames) {
+        F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+        if (DISubprogram *SP = F.getSubprogram()) {
----------------
What's this about/for? Should this be in an independent change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656