Page MenuHomePhabricator

[PGO] Directory name stripping in global identifier for static functions
ClosedPublic

Authored by xur on Feb 3 2017, 1:25 PM.

Details

Summary

Current internal option -static-func-full-module-prefix strips all the directory path the profile counter names for static functions. The default of this option is true. This is problematic:

(1) it creates linker errors for profile-generation compilation, exposed in our
internal benchmarks. We are seeing messages like
"warning: relocation refers to discarded section".
This is due to the name conflicts after the stripping.

(2) the stripping only applies to getPGOFuncName. This can create inconsistency in thin-lto's static promotion, which in turn causes problems in thin-lto's indirect-call-promotion (missing targets).

This patch turns the default value for -static-func-full-module-prefix to false.

The second part of the patch is to have an alternative implementation under the internal option -static-func-strip-dirname-prefix=<value>

This options specifies level of directories to be stripped from the source
filename. Using a large value as the parameter has the same effect as
-static-func-full-module-prefix.

We plan to retire -static-func-full-module-prefix in the future.

Diff Detail

Repository
rL LLVM

Event Timeline

xur created this revision.Feb 3 2017, 1:25 PM
tejohnson edited edge metadata.Feb 3 2017, 1:35 PM

(2) the stripping only applies to getPGOFuncName. This can create inconsistency in thin-lto's static promotion, which in turn causes problems in thin-lto's indirect-call-promotion (missing targets).

Slight correction - it doesn't affect ThinLTO static promotion (which uses the module hash not the guid). It affects the ThinLTO indirect call promotion since the PGO name needs to match the guid used in the index.

lib/IR/Globals.cpp
38 ↗(On Diff #87009)

It isn't just profile counter names. "in the global identifier for static functions".

lib/ProfileData/InstrProf.cpp
31 ↗(On Diff #87009)

Should this one be removed, is it useful with the new option?

xur added a comment.Feb 3 2017, 1:59 PM

thanks to Teresa for the collection. I'll update the patch shortly

lib/IR/Globals.cpp
38 ↗(On Diff #87009)

Fixed.

lib/ProfileData/InstrProf.cpp
31 ↗(On Diff #87009)

This will be removed later as we assume this options is being used by the users.

xur updated this revision to Diff 87021.Feb 3 2017, 2:01 PM

updated the option description per Teresa's suggestion.

davidxl added inline comments.Feb 3 2017, 3:51 PM
lib/IR/Globals.cpp
125 ↗(On Diff #87021)

This is PGO specific, probably move the change into InstrProf.cpp including the option etc.

128 ↗(On Diff #87021)

format fix.

159 ↗(On Diff #87021)

getGlobalIdentifier is a low level interface. Move this into getPGOFuncName

test/Transforms/PGOProfile/statics_counter_naming.ll
4 ↗(On Diff #87021)

The test does not actually test anything. Better add case that strips the expected separators.

tejohnson added inline comments.Feb 3 2017, 4:00 PM
lib/IR/Globals.cpp
159 ↗(On Diff #87021)

Rong and I discussed this - putting it here ensures the same naming scheme is used for PGO and for the ThinLTO index. Although for ThinLTO we will likely not want this enabled. I'm not sure if there is a good way to enforce that.

xur added inline comments.Feb 3 2017, 4:43 PM
test/Transforms/PGOProfile/statics_counter_naming.ll
4 ↗(On Diff #87021)

I can not assume the source directory structure. It's hard to write a test that strip the exact number of directory strings. But at least this test shows that the new option can replace the original option.

davidxl added inline comments.Feb 3 2017, 5:14 PM
test/Transforms/PGOProfile/statics_counter_naming.ll
4 ↗(On Diff #87021)

ok. perhaps add a negative test that if stripping is not used, it will fail?

silvas edited edge metadata.Feb 3 2017, 8:31 PM

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).

From the description:

Current internal option -static-func-full-module-prefix strips all the directory path the profile counter names for static functions. The default of this option is true.

...

This patch turns the default value for -static-func-full-module-prefix to false.

However in the diff I see:

  • "static-func-full-module-prefix", cl::init(false),

+ "static-func-full-module-prefix", cl::init(true),

Which is the opposite of the description? This seems confusing to me.

xur updated this revision to Diff 89390.Feb 22 2017, 11:37 AM

Update the patch based on the reviews:
(1) move the code to ProfileData to make the change local to profile
(2) add negative test
(3) add a comment on how it affects thin-lto.

xur added a comment.Feb 22 2017, 11:42 AM

From the description:

Current internal option -static-func-full-module-prefix strips all the directory path the profile counter names for static functions. The default of this option is true.

...

This patch turns the default value for -static-func-full-module-prefix to false.

However in the diff I see:

  • "static-func-full-module-prefix", cl::init(false), + "static-func-full-module-prefix", cl::init(true),

    Which is the opposite of the description? This seems confusing to me.

Sorry for the confusion. It seems my description was not accurate.

The current default value for -static-func-full-module-prefix is false, which means it strips all the path names.

This patch will make the default value as true, which keeps the path names.

This revision is now accepted and ready to land.Feb 24 2017, 10:56 AM
This revision was automatically updated to reflect the committed changes.