This is an archive of the discontinued LLVM Phabricator instance.

Remove the restriction that known and unknown probabilities cannot coexist when being normalized.
ClosedPublic

Authored by congh on Dec 15 2015, 4:26 PM.

Details

Summary

The current BranchProbability::normalizeProbabilities() forbids known and unknown probabilities to coexist in the list. This was once used to help capture probability exceptions but has caused some reported build failures (https://llvm.org/bugs/show_bug.cgi?id=25838).

This patch removes this restriction by evenly distributing the complement of the sum of all known probabilities to unknown ones. We could still treat this as an abnormal behavior, but it is better to emit warnings in our future profile validator.

Diff Detail

Event Timeline

congh updated this revision to Diff 42936.Dec 15 2015, 4:26 PM
congh retitled this revision from to Remove the restriction that known and unknown probabilities cannot coexist when being normalized..
congh updated this object.
congh added a reviewer: davidxl.
congh added a subscriber: llvm-commits.
davidxl edited edge metadata.Dec 16 2015, 10:01 AM

While it makes sense to do this -- can we start to teach producers not to pass unknown Probabilities in the first place?

While it makes sense to do this -- can we start to teach producers not to pass unknown Probabilities in the first place?

If you search addSuccessor in LLVM, you will find about 200 users for different platforms without probability passed explicitly. Some are conditionally used such as:

MBB->addSuccessor(Succ1);
if (...)

MBB->addSuccessor(Succ2);

It seems that a default probability is more convenient but I don't object to add probabilities explicitly. There are just some tradeoffs.

davidxl added inline comments.Dec 17 2015, 1:54 PM
include/llvm/Support/BranchProbability.h
194

why guarded with Sum > 0?

200

may be name it 'ProbForUnknown'

congh marked an inline comment as done.Dec 17 2015, 2:12 PM
congh added inline comments.
include/llvm/Support/BranchProbability.h
194

After reviewing the patch, I think this guard should be removed. Thanks for pointing it out!

congh updated this revision to Diff 43182.Dec 17 2015, 2:13 PM
congh edited edge metadata.

Update the patch on David' comment.

davidxl accepted this revision.Dec 17 2015, 2:19 PM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Dec 17 2015, 2:19 PM
This revision was automatically updated to reflect the committed changes.