This is an archive of the discontinued LLVM Phabricator instance.

[PGO]Implement metadata combine for 'branch_weights' of direct callsites when none of the instructions folds the rest away
ClosedPublic

Authored by mingmingl on Apr 20 2023, 11:45 PM.

Details

Summary

Diff Detail

Event Timeline

mingmingl created this revision.Apr 20 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 11:45 PM
mingmingl requested review of this revision.Apr 20 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 11:45 PM
nikic added a subscriber: nikic.Apr 21 2023, 1:21 AM
nikic added inline comments.
llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll
1

You need --check-globals to check the resulting metadata. Also, it should be possible to reduce these tests to basically an if/else with a call each.

mingmingl updated this revision to Diff 515631.Apr 21 2023, 8:39 AM
mingmingl edited the summary of this revision. (Show Details)
mingmingl marked an inline comment as done.
mingmingl edited the summary of this revision. (Show Details)

resolve comments.

davidxl added inline comments.Apr 21 2023, 10:25 AM
llvm/include/llvm/IR/Metadata.h
1322

Have an one line summary of what the interface does here. Move the detailed documentation to the function definition.

llvm/lib/IR/Metadata.cpp
1075

Why not naming it 'mergeDirectCallProfMetadata'? Even if there is a need to merge indirect call profiles, the implementation will be quite different and may require a different method for it.

1078

Combine these two into if (! (A&&B)) return;

1101

how about == 2?

1116

why is the check needed?

mingmingl marked 4 inline comments as done.

resolve comments.

This revision is now accepted and ready to land.Apr 21 2023, 11:38 AM
mingmingl marked an inline comment as done.Apr 21 2023, 11:42 AM
mingmingl added inline comments.
llvm/lib/IR/Metadata.cpp
1075

Even if there is a need to merge indirect call profiles, the implementation will be quite different and may require a different method for it.

This is true. Here either the caller 'combineMetadata' or callee ('getMergedProfMetadata' or 'mergeDirectCallProfMetadata') needs to look at 'AInstr' or 'BInstr' to decide if it's the implemented case, and it looks cleaner (in the caller) by putting details in the callee 'getMergedProfMetadata'

Added a private method 'mergeDirectCallProfMetadata' to make future expansions easier.

1101

'== 2' works for 'callinstr' with 'branch_weights' (e.g., after seeing AProfName.equals("branch_weights") && BProfName.equals("branch_weights")) -> yet !prof for direct callsites might be expanded in the future (like invoke prof could have both branch weights and value profiles) so here make assertions closer what LLVM verifier already verifies.

1116

Changed it to assert since verifier already has Check(mdconst::dyn_extract<ConstantInt>(MDO), "!prof brunch_weights operand is not a const int");

llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll
1

thanks! Reduced the test case and added '-check-globals'

nikic added inline comments.Apr 21 2023, 11:55 AM
llvm/lib/Transforms/Utils/Local.cpp
2721

This should probably be behind a DoesKMove check? If we're CSEing with a dominating call, it doesn't make sense to add the branch weights, as the second call is being removed.

I'd suggest adding a test with a readonly/nounwind/willreturn call being GVNed to cover that case.

davidxl added inline comments.Apr 21 2023, 12:14 PM
llvm/lib/Transforms/Utils/Local.cpp
2721

good catch.

mingmingl planned changes to this revision.Apr 21 2023, 1:16 PM
mingmingl marked an inline comment as done.

plan changes as suggested.

llvm/lib/Transforms/Utils/Local.cpp
2721

thanks for the catch!

Just to confirm I'm getting the point (not particular to a specific pass yet), in https://gcc.godbolt.org/z/nGa6YqWdP, two calls to 'func1' in line 11 and line14 should keep one branch weight (rather than the sum of two). So that is the case when unifying (not merge) is correct.

nikic added inline comments.Apr 21 2023, 2:09 PM
llvm/lib/Transforms/Utils/Local.cpp
2721

Yeah, that's the case I had in mind. After the transform only the first call will be executed, so it would make sense to me to only keep its call count.

mingmingl updated this revision to Diff 517320.Apr 26 2023, 2:36 PM
mingmingl marked 3 inline comments as done.
mingmingl retitled this revision from [PGO]Implement metadata combine for 'branch_weights' of direct callsites to [PGO]Implement metadata combine for 'branch_weights' of direct callsites when none of the instructions folds the rest away.
mingmingl edited the summary of this revision. (Show Details)

gate the sum of branch weights by DoesKMove and add test case when one callsite folds away (GVN'ed the other)

This revision is now accepted and ready to land.Apr 26 2023, 2:36 PM
mingmingl added inline comments.Apr 26 2023, 2:36 PM
llvm/lib/Transforms/Utils/Local.cpp
2721

https://gcc.godbolt.org/z/9cnd3qEce is a test case when EarlyCSE CSE'ed call to 'func1', while turns out the current implementation doesn't invoke merge-metadata around where calls are CSE'ed (probably goes to a follow-up patch)

Modified existing test case in GVN.

This revision was landed with ongoing or failed builds.Apr 27 2023, 1:05 PM
This revision was automatically updated to reflect the committed changes.