[PATCH] D29512: [PGO] Directory name stripping in global identifier for static functions

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

This change does two things (as you mention in the description):

1. Adding -static-func-strip-dirname-prefix which provides a way to have more control when `-static-func-full-module-prefix=true` is specified.
2. Changing the default of -static-func-full-module-prefix to true.

IIRC, -static-func-full-module-prefix defaults to false because it caused issues when set to true (in fact, it was introduced to avoid these issues). The default value of -static-func-strip-dirname-prefix introduced in this patch (i.e. 0) is effectively a no-op; so ignore 1. for now. This means that the net effect of this patch is that compilation will, by default, have a regression on the issue fixed by r275193 / http://reviews.llvm.org/D22028, which is not a good idea. I think that the default behavior (which is user-visible) should not be changed in this patch.

Overall, it sounds like this approach of relying on users to tweak internal compiler options (-mllvm) to get correct behavior in their environment is not the kind of user experience we want to deliver (or the kind of implementation that we want to maintain). IIRC, when we added -static-func-full-module-prefix, it was with the understanding that it was a simple hack for working around the larger issue of relying on the module name which we knew was not very robust. The further addition of the "InLTO" complicates things even further. It seems like a code smell that we do not have a Single Point Of Truth.

I proposed a solution at one point https://groups.google.com/d/msg/llvm-dev/s_VZbFTWbVs/d0b4Zh80CgAJ though it may no longer be applicable. It seems like ThinLTO already has to solve a problem of finding unique identifiers for all functions (even static), so we may want to piggy-back on that mechanism (this is just a high-level thought; haven't looked into the details).

So:

- I specifically object to changing user-visible defaults in this patch. Those changes should be isolated, and I don't think we have justification to change those defaults anyway.
- I'm slightly opposed to adding the -static-func-strip-dirname-prefix flag, since it seems like a workaround (among others that have already piled up) for a more fundamental issue. This is a frog-in-boiling-water situation; if solving the fundamental issue would be a huge amount of work, then adding the new flag is probably fine for now, but we need to keep in mind the larger situation. IIUC, defaulting `-static-func-strip-dirname-prefix=-1` would emulate the current default behavior, so -static-func-full-module-prefix could just be removed in the same patch.
- I would encourage brainstorming/discussion of alternative solutions that solve the fundamental problem (which seems to be more about having a stable globally unique identifier than being specifically about preserving/mangling the "name" per se).




https://reviews.llvm.org/D29512