This is an archive of the discontinued LLVM Phabricator instance.

Add a pass to name anonymous/nameless function
ClosedPublic

Authored by mehdi_amini on Apr 7 2016, 5:34 PM.

Details

Summary

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.

Diff Detail

Event Timeline

mehdi_amini updated this revision to Diff 52985.Apr 7 2016, 5:34 PM
mehdi_amini retitled this revision from to Add a pass to name anonymous/nameless function.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added a subscriber: llvm-commits.

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?

tejohnson edited edge metadata.Apr 8 2016, 6:55 AM

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:
Twine("anon.") + ModuleHash + "." + Twine(count++)

mehdi_amini added inline comments.Apr 8 2016, 7:41 AM
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.

tejohnson added inline comments.Apr 8 2016, 7:52 AM
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:
Twine("anon.") + ModuleHash + Twine(".") + Twine(count++)

mehdi_amini added inline comments.Apr 8 2016, 9:01 AM
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

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.

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.

mehdi_amini updated this revision to Diff 53034.Apr 8 2016, 9:46 AM
mehdi_amini edited edge metadata.

Address comment (style + Twine bugfix + lazy compute hash).

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.
mehdi_amini marked 12 inline comments as done.Apr 12 2016, 1:54 PM

Ping? I think I addressed all the comments so far?

tejohnson accepted this revision.Apr 12 2016, 2:04 PM
tejohnson edited edge metadata.

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.

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.

This revision is now accepted and ready to land.Apr 12 2016, 2:04 PM

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.

In D18883#399035, @joker.eph wrote:

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.

Sounds good.

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?

It should already be fixed with r266151

mehdi_amini closed this revision.Apr 13 2016, 11:59 AM