This is an archive of the discontinued LLVM Phabricator instance.

[Propeller] Match debug info filenames from profiles to distinguish internal linkage functions with the same names.
ClosedPublic

Authored by rahmanl on Mar 23 2023, 4:19 PM.

Details

Summary

Basic block sections profiles are ingested based on the function name. However, conflicts may occur when internal linkage functions with the same symbol name are linked into the binary (for instance static functions defined in different modules). Currently, these functions cannot be optimized unless we use -funique-internal-linkage-names (D89617) to enforce unique symbol names.

However, we have found that -funique-internal-linkage-names does not play well with inline assembly code which refers to the symbol via its symbol name. For example, the Linux kernel does not build with this option.

This patch implements a new feature which allows differentiating profiles based on the debug info filenames associated with each function. When specified, the given path is compared against the debug info filename of the matching function and profile is ingested only when the debug info filenames match. Backward-compatibility is guaranteed as omitting the specifiers from the profile would allow them to be matched by function name only. Also specifiers can be included for a subset of functions only.

Diff Detail

Event Timeline

rahmanl created this revision.Mar 23 2023, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 4:19 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
rahmanl updated this revision to Diff 507916.Mar 23 2023, 4:19 PM

Add test.

rahmanl updated this revision to Diff 509827.Mar 30 2023, 2:50 PM

Add test cases.

rahmanl updated this revision to Diff 512691.Apr 12 2023, 1:53 AM
  • Implement selective profile reading. Profile for each function is only retrieved if it corresponds to a function in the current module.
rahmanl updated this revision to Diff 512701.Apr 12 2023, 2:13 AM
  • Implement selective profile reading. Profile for each function is only retrieved if it corresponds to a function in the current module.
rahmanl updated this revision to Diff 512706.Apr 12 2023, 2:22 AM
  • Cleanup.
rahmanl retitled this revision from Read module names from Propeller profile. to [Propeller] Read debug info filenames from the basic block sections profile to distinguish internal linkage functions with the same names..Apr 12 2023, 2:34 PM
rahmanl edited the summary of this revision. (Show Details)
rahmanl added reviewers: tmsriram, shenhan, xur, tejohnson, amharc.
rahmanl published this revision for review.Apr 12 2023, 3:07 PM
rahmanl retitled this revision from [Propeller] Read debug info filenames from the basic block sections profile to distinguish internal linkage functions with the same names. to [Propeller] Match debug info filenames from profiles to distinguish internal linkage functions with the same names..
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 3:07 PM
rahmanl edited the summary of this revision. (Show Details)Apr 12 2023, 3:16 PM
rahmanl edited the summary of this revision. (Show Details)Apr 12 2023, 3:16 PM

Just to add to the limitations of -funique-internal-linkage-functions, even the alias attribute can cause problems. Any attribute that takes the name of a function as a string, alias / asm, will not work with this option as it involves changing the source. The kernel has plenty of these and hence we need a better solution.

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
47

Isn't this a refactoring change that should go into a separate patch?

60

Should the comments be changed to reflect how module names can be specified?

95

How is this valid now?

132

Here somehow, can you get the linkage type of Alias and discard cases where it is a non internal linkage function? I haven't thought of how to do it yet.

135

First of all, you shouldnt be checking DIFilename.empty() here, that case must be dismissed much earlier above. If DIFilename is empty, simply don't check anything as this is equal to doing the default and do it immediately after getting DIFilename above.

The condition must also look like this:

return It != FunctionNameToDIFilename.end() ? It->second.equals(DIFilename) : false;

If It->second is empty and we have specified a module, we should conclude false, WDYT?

149

Why would this be problem now and not earlier? Add a comment to make it clear how duplicate function names could happen.

166

Not a great optimization but only internal linkage function definitions here really matter right?

I am not sure but it appears to me that you are storing every defined function name in the map to make the other check work. That is not wrong but you may be able to do something better.

Here is an idea:
Store only internal linkage functions in the map. If you cannot find it in the map, then clearly it is not an internal linkage function name corresponding to this module and its cluster info can be safely discarded. However, there is an argument that global linkage functions can also have their modules specified but should be made beyond the scope of this feature. WDYT?

llvm/test/CodeGen/X86/basic-block-sections-clusters-error.ll
21

Ah okay, that's how you get duplicate profiles. Understood but please make sure this is documented in the code above. Also, this case could have occurred even before where you specify multiple profiles for the same function.

rahmanl marked 5 inline comments as done.Apr 18 2023, 2:31 PM

Thanks for the quick feedback @tmsriram

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
47
95

I silenced this error handling to allow the condition to imply that the profile must be skipped.

132

Yes. This is doable in the doInitialization call. However, I am not sure if we should discard these cases. My point is "if the debug-info-filename is specified, we should require that it is matched. If it is omitted, then it will be matched with anything".

135

This is a bit subtle. If debug-info-filename specifier is empty, we should still check that the function can be found in the module. This won't change the final result, but it makes the internal logic consistent with the case when debug-info-filename is specified (where both function name and debug-info filename are checked).

166
If you cannot find it in the map, then clearly it is not an internal linkage function name corresponding to this module and its cluster info can be safely discarded.

But we are not supposed to discard the profiles for global linkage functions. right?

rahmanl updated this revision to Diff 514747.Apr 18 2023, 2:31 PM
rahmanl marked 2 inline comments as done.
  • address comments.
rahmanl updated this revision to Diff 522292.May 15 2023, 12:15 PM
  • Extract filename via DICompileUnits. Match on filename attribute only.

Thanks for implementing this.

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
124

nit: backslash can be dropped in "Unknown string found: \'".

151–152

Do you think we shall make this more tolerant? Instead of returning a invalidProfileError, just drop the profile for this specific function?

164

Could you spell out the full type?

172

I'll apply this to create_llvm_prof too.

178–179

This mean there are no function definitions in the module? Is it an error?
(If it is a real error, we can make it more meaningful, like "module does not contain function defintions, etc...")

rahmanl updated this revision to Diff 524858.May 23 2023, 1:20 PM
rahmanl marked 4 inline comments as done.
  • Address reviewer comments.
llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
151–152

I suggest we have a hard failure in such cases. Silently dropping such cases may lead to uncaught performance drops. However, avoiding duplicates is something that the profile processing tool can easily do.

178–179

Removed. It was debug printing.

rahmanl updated this revision to Diff 524862.May 23 2023, 1:27 PM
  • rebase.
shenhan accepted this revision.Jun 9 2023, 9:56 AM

Locally tested the patch.

LGTM

This revision is now accepted and ready to land.Jun 9 2023, 9:56 AM