This is an archive of the discontinued LLVM Phabricator instance.

[BrachProbablityInfo] Proportional distribution of reachable probabilities
ClosedPublic

Authored by yrouban on May 26 2020, 10:15 PM.

Details

Summary

When fixing probability of unreachable edges in BranchProbabilityInfo::calcMetadataWeights() proportionally distribute remainder probability over the reachable edges. The old implementation distributes the remainder probability evenly. See examples in the fixed tests.

Diff Detail

Event Timeline

yrouban created this revision.May 26 2020, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 10:15 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ebrevnov added inline comments.May 26 2020, 10:58 PM
llvm/lib/Analysis/BranchProbabilityInfo.cpp
423

It would help to understand the code if you split this series of calculations:

uint64_t Mul = static_cast<uint64_t>(...)
uint32_t Div = static_cast<uint_32_t>l(lvm::divideNearest(...));
BP[i] = BranchProbability::getRaw(Div);

hjyamauchi added inline comments.May 27 2020, 11:35 AM
llvm/lib/Analysis/BranchProbabilityInfo.cpp
105

"equally" -> "proportionally"?

371

'independed on' -> 'independent of'?

384

Capitalize 'i' -> 'I'.

This loop could be combined with the above loop, but would be less clear?

391

Capitalize 'i' -> 'I'

yrouban marked 5 inline comments as done.May 27 2020, 11:44 PM
yrouban added inline comments.
llvm/lib/Analysis/BranchProbabilityInfo.cpp
384

The old code above has the index i lowcased. I would propose to keep the style.
Yes, the separate loop is for clarity.

391

ditto, if you do not insist

yrouban updated this revision to Diff 266755.May 27 2020, 11:45 PM

addressed comments.

hjyamauchi accepted this revision.May 29 2020, 10:05 AM
hjyamauchi added inline comments.
llvm/lib/Analysis/BranchProbabilityInfo.cpp
384

OK, though in general I'd encourage to update to the capitalized variable name style in newly added code.

This revision is now accepted and ready to land.May 29 2020, 10:05 AM
yrouban marked an inline comment as done.Jun 1 2020, 9:19 PM

This seems to have been pushed to a branch called D80611-land2 and not to trunk.

This seems to have been pushed to a branch called D80611-land2 and not to trunk.

Sorry, I used a wrong push command. I will land it soon.

This revision was automatically updated to reflect the committed changes.