Page MenuHomePhabricator

[Instruction] Remove setProfWeight()
ClosedPublic

Authored by yrouban on Jun 2 2020, 2:09 AM.

Details

Summary

Remove the function Instruction::setProfWeight() and make use of Instruction::copyMetadata(.., {LLVMContext::MD_prof}).
This is correct for all use cases of setProfWeight() as it is applied to CallBase instructions only.
This change results in prof metadata copied intact even if the source has "VP". The old pair of calls extractProfTotalWeight() + setProfWeight() resulted in setting branch_weights if the source had "VP" data.
This is a split part of D80618.

Diff Detail

Event Timeline

yrouban created this revision.Jun 2 2020, 2:09 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
davidxl added inline comments.Jun 2 2020, 9:10 AM
llvm/test/Transforms/InstCombine/cast-call-combine-prof.ll
35

where does 'branch_weights' profdata go?

yrouban updated this revision to Diff 268059.Jun 2 2020, 10:27 PM

no changes but added wider context for ease of review

yrouban marked an inline comment as done.Jun 2 2020, 10:29 PM
yrouban added inline comments.
llvm/test/Transforms/InstCombine/cast-call-combine-prof.ll
35

"VP" is preserved as pointed out in the summary.

yamauchi added inline comments.Jun 3 2020, 9:05 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4957–4958

This refers to SamplePGO preserving the call count in the metadata (which originates in D31344), but it seems more general as it could also apply to calls under PGO (eg the VP metadata).

davidxl added inline comments.Jun 3 2020, 9:05 AM
llvm/test/Transforms/InstCombine/cast-call-combine-prof.ll
35

If the invoke is still there, the branch weights should also be copied over, right?

yrouban marked an inline comment as done.Jun 3 2020, 9:22 PM
yrouban added inline comments.
llvm/test/Transforms/InstCombine/cast-call-combine-prof.ll
35

In cast-call-combine-prof.ll there is no branch_weights metadata.
As the instcombine is fixed it is not converting "VP" to "branch_weights".
If the invoke instruction had "branch_weights" then the fixed instcombine would preserve it as well. In other words, with this patch we just copy whatever !prof data we have.
!prof can have either "VP" or "branch_weights", but not both.

davidxl accepted this revision.Jun 3 2020, 9:32 PM

lgtm (after fixing the comment mentioned by Hiroshi)

llvm/test/Transforms/InstCombine/cast-call-combine-prof.ll
35

ah thanks. The old diff does not have context so I read it wrong

This revision is now accepted and ready to land.Jun 3 2020, 9:32 PM
This revision was automatically updated to reflect the committed changes.