For correct handling of alias to nameless
function, we need to be able to refer them through a GUID in the summary.
Here we name them using a hash of the non-private global names in the module.
Details
- Reviewers
tejohnson - Commits
- rGd5faa267c4a7: Add a pass to name anonymous/nameless function
Diff Detail
Event Timeline
Hi! Just a few comments in passing for you. :)
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
31 | Nit: Can this just be folded into if (F.isDeclaration() || F.hasLocalLinkage() || !F.hasName())? | |
41 | And this | |
59 | ISTM this code is making many Twines here, and the lifetimes of all but the top-level Twine will end when we're done constructing Prefix, which is bad. Am I missing something? |
Perhaps this should be named something more general than NameAnonFunctionPass. This is the naming scheme pcc was proposing for promoted values as well, and eventually it would be good to do any early renaming/promotion of locals here. The moduleHash() facility could also be used during importing to rename/promote imported locals (i.e. invoked from FunctionImportGlobalProcessing), for anything not promoted/renamed during the compile step, so it would also be good to put that into a more general location.
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
59 | Right, I think this should just be std::string (although the Twine(count++) below seems fine since it is constructing a function parameter). | |
64 | Another possibility would be to move the Twines here: |
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
31 | Thanks, good point, forgot to clean this up... | |
59 | George: I don't see what is the issue here: the Twine are *copied*. The lifetime we have to look at is the lifetime of the underline objects every Twine wraps. Teresa: this is a good suggestion in general: serializing the prefix once in a string here should be better than doing it multiple time in the loop as part of the name. The catch here is that I don't expect the loop to be large in average (is unnamed function a common thing?), and serializing early means unconditionally (not that I think it matters here...). | |
64 | I believe this is NFC, but loses the "string optimization" you mentioned above. |
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
59 | Reading through http://llvm.org/docs/ProgrammersManual.html#dss-twine, I am not convinced the lifetime of the "anon." temporary will be extended. If it is legal, why wrap the "anon." part in a Twine and not "."? We're already doing the expensive part (computation of the ModuleHash string) unconditionally, I'm not sure it is worth trying to optimize this part. Especially since the Twine usage makes me nervous, but I could be missing something... | |
64 | Actually, this could be: |
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
59 | You're asking why "anon." is wrapped in a Twine and not ".", this is because otherwise RHS would not operate on Twine at all. And you don't need to wrap "." in a Twine, because it is already implicitly done by the compiler! (here is the clang-ast if you're interested: http://snag.gy/HtMMD.jpg ) That said, with another look at it, George is right: the problem is not the lifetime of the "anon." temporary, the problem is the lifetime of the intermediate Twine objects. The expression I wrote is conceptually the same as: Twine Prefix; { auto TwineAnon = Twine("anon."); auto TwineModuleHash = Twine(ModuleHash); auto TwineAnonAndModuleHash = TwineAnon + TwineModuleHash; auto TwineDot = Twine("."); Prefix = TwineAnonAndModuleHash + TwineDot;` } // at scope exit the intermediate Twine are dead but Prefix keeps pointers to them // the underlying objects "anon.", ModuleHash, and "." are still valid. | |
59 | (Also we may not want to compute the hash unconditionally) | |
64 | There is no difference with your previous suggestion (the Twine is implicit for "."). |
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
59 |
I agree with Teresa, because when hooking a Twine up to another Twine, we call Twine::concat, which stores the this pointer (and the pointer to the concat argument) in the newly-created Twine. Given that a single Twine can only hook two things together, we'll need at least two of them here to hook all three parts of this string together. |
Somehow only one of my previous inline comment went through?
lib/Transforms/Utils/NameAnonFunctions.cpp | ||
---|---|---|
59 | You're asking why "anon." is wrapped in a Twine and not ".", this is because otherwise RHS would not operate on Twine at all. And you don't need to wrap "." in a Twine, because it is already implicitly done by the compiler! (here is the clang-ast if you're interested: http://snag.gy/HtMMD.jpg ) That said, with another look at it, George is right: the problem is not the lifetime of the "anon." temporary, the problem is the lifetime of the intermediate Twine objects. The expression I wrote is conceptually the same as: Twine Prefix; { auto TwineAnon = Twine("anon."); auto TwineModuleHash = Twine(ModuleHash); auto TwineAnonAndModuleHash = TwineAnon + TwineModuleHash; auto TwineDot = Twine("."); Prefix = TwineAnonAndModuleHash + TwineDot;` } // at scope exit the intermediate Twine are dead but Prefix keeps pointers to them // the underlying objects "anon.", ModuleHash, and "." are still valid. |
I had the above comment. However, I don't have strong objections to putting this in as-is. It could be refactored or renamed later to use the same naming facility for early promotion/renaming.
I missed this comment originally.
So the pass itself is really intended to be targeting nameless functions. The ModuleHash computation could be refactored though (I'll leave it for when needed).
I'm not sure how to write the llvm::nameUnamedFunctions() to be reusable though. It should be easier to figure out when the exact need will be there.
It seems this CL is breaking LLDB Windows builder - http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/5228, could you take a look?
Nit: Can this just be folded into if (F.isDeclaration() || F.hasLocalLinkage() || !F.hasName())?