This is an archive of the discontinued LLVM Phabricator instance.

Let SelectionDAG start to use probability-based interface to add successors.
ClosedPublic

Authored by congh on Nov 4 2015, 4:58 PM.

Details

Summary

The patch in http://reviews.llvm.org/D13745 is broken into four parts:

  1. New interfaces without functional changes.
  2. Use new interfaces in SelectionDAG, while in other passes treat probabilities as weights.
  3. Use new interfaces in all other passes.
  4. Remove old interfaces.

This the second patch above. In this patch SelectionDAG starts to use probability-based interfaces in MBB to add successors but other MC passes are still using weight-based interfaces. Therefore, we need to maintain correct weight list in MBB even when probability-based interfaces are used. This is done by updating weight list in probability-based interfaces by treating the numerator of probabilities as weights. This change affects many test cases that check successor weight values. I will update those test cases once this patch looks good to you.

Diff Detail

Repository
rL LLVM

Event Timeline

congh retitled this revision from to Let SelectionDAG start to use probability-based interface to add successors..Nov 4 2015, 4:58 PM
congh updated this revision to Diff 39293.Nov 4 2015, 4:58 PM
congh updated this object.
congh added reviewers: dexonsmith, manmanren, davidxl.
congh added a subscriber: llvm-commits.
manmanren edited edge metadata.Nov 6 2015, 8:30 PM

It seems that we can submit the changes other than SelectionDAGBuilder.h, SelectionDAGBuilder.cpp and SelectionDAGISel.cpp, separately. Correct me if I am wrong.

Thanks for working on this!
Manman

lib/CodeGen/MachineBasicBlock.cpp
534 ↗(On Diff #39293)

typo: replaced
Thanks for doing extra to make code review easier!

565 ↗(On Diff #39293)

I don't quite get why we need to comment out this. When we support both weights and branch probabilities, should we keep both? We can remove weights in the end, right?

lib/Target/PowerPC/PPCISelLowering.cpp
8425 ↗(On Diff #39293)

This change seems to be different from the weight distribution before. Can you add comments on why?

davidxl edited edge metadata.Nov 7 2015, 4:10 PM

How many more weight based uses are still remaining after this patch? If there are only a few, consider move those uses into this patch too. It is a weird state that both weights and probs exists (and used at the same time).

include/llvm/Support/BranchProbability.h
95 ↗(On Diff #39293)

Saturation due to rounding errors is fine, but it seems we may need a way to differentiate the cases where true user errors we made.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1499 ↗(On Diff #39293)

Use a helper interface in MachineBasicBlock?

1613 ↗(On Diff #39293)

The Prob distribution for TemBB does not look correct --- they don't sum up to 1.

It should be A/(A+2B), 2B/(A+2B).

It is probably useful to introduce probability division interface.

1835 ↗(On Diff #39293)

can you add more comments describing the lowered CFG and explain why the profile is updated this way?

1842 ↗(On Diff #39293)

Why can CB.TrueProb be zero and be different from getEdgeProbability(SwitchBB, CB.TrueBB) ?

1847 ↗(On Diff #39293)

Why they don't sum up to one and needs normalization? Is the problem in the caller?

congh marked an inline comment as done.Nov 9 2015, 11:42 AM

How many more weight based uses are still remaining after this patch? If there are only a few, consider move those uses into this patch too. It is a weird state that both weights and probs exists (and used at the same time).

There are passes like IfConversion, BranchFolding, MachineBlockPlacement, TailDuplication, and MIParser that are using those interfaces. This patch is our planned step two, in which only SelectionDAGBuilder is using the new interfaces. Maybe we want to combine step 2 and 3?

include/llvm/Support/BranchProbability.h
95 ↗(On Diff #39293)

So is it better to remove the rounding behavior when constructing BP?

lib/CodeGen/MachineBasicBlock.cpp
565 ↗(On Diff #39293)

As the BP-based new interfaces are now only used in SelectionDAGBuilder, and later passes are still using old weight-based interfaces, it is possible that a later pass adds a successor with only weight and then removes this successor, which doesn't have any probability stored. Yes, we will remove all weight-based interfaces in the end.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1499 ↗(On Diff #39293)

SrcBB is a BB not a MBB. I think we don't want to change BB right?

1613 ↗(On Diff #39293)

As A+B=1, A/(1+B) + 2B(1+B) = (A+2B)/(1+B) = (1+B)/(1+B)=1.

I doubt if the division between BP that is defined in BP is useful: we may frequently get a result that is larger than one. Fortunately, it seems we may only need this operation here.

1842 ↗(On Diff #39293)

Previously, CB.TrueWeight can also be zero, in which case getEdgeWeight() is also called, which is wrapped in addSuccessorWithWeight. As now we have normalizeSuccProbs() in MBB, I have update this part by using it.

1847 ↗(On Diff #39293)

The calculation of CB.TrueProb and CB.FalseProb cannot guarantee that their sum is one. The existed code is for calculating weights which only keeps correct relative relations of CB.True/FalseProb. And in this file there are several other places in which we need to normalize a group of probabilities.

lib/Target/PowerPC/PPCISelLowering.cpp
8425 ↗(On Diff #39293)

This is due to an old assumption that the edge weight should not be zero. Actually there is a bug here: the zero weight will be converted to a default weight 16 later. But now as we can use zero probability, I will update it by passing 0 and 1 probability.

congh added a comment.Nov 9 2015, 11:50 AM

It seems that we can submit the changes other than SelectionDAGBuilder.h, SelectionDAGBuilder.cpp and SelectionDAGISel.cpp, separately. Correct me if I am wrong.

Thanks for working on this!
Manman

Changes under lib/Target/ should be checked in with SelectionDAG* as they are all doing instruction lowering. Other changes are for them so I think we need to put them together.

davidxl added inline comments.Nov 9 2015, 1:43 PM
include/llvm/Support/BranchProbability.h
95 ↗(On Diff #39293)

you mean always round to zero? probably not a good idea.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1499 ↗(On Diff #39293)

Can probably be added when weight based interrfaces for BB are changed later.

1613 ↗(On Diff #39293)

You are right -- A/(A+2B) actually = A/(A+B +B ) = A/(1+B), so your is correct.

1624 ↗(On Diff #39293)

Would it be better to use a new variable TmpTrueProb here? (similarly TmpFalseProb)?

1632 ↗(On Diff #39293)

It seems more readable to set TmpBB's prob to {A/2, B}, and then later normalize it using the normalization interface.

1660 ↗(On Diff #39293)

Similarly here.

1842 ↗(On Diff #39293)

Is it safe to unconditionally use getEdgeProbability()? will it return different value?

1847 ↗(On Diff #39293)

Can you point out the those lines in the file that assign non-normalized PBs?

It seems that we can submit the changes other than SelectionDAGBuilder.h, SelectionDAGBuilder.cpp and SelectionDAGISel.cpp, separately. Correct me if I am wrong.

Thanks for working on this!
Manman

Changes under lib/Target/ should be checked in with SelectionDAG* as they are all doing instruction lowering. Other changes are for them so I think we need to put them together.

I am kind of confused. It seems to me that this change only sets up Probs array in MachineBasicBlock, but it does not start using it. When looking for getProbabilityIterator or getSuccProbability, I can't find any usage.

If we commit the change in lib/Target/X86/X86FrameLowering.cpp, we will be calling the new interface addSuccor(..., BranchProb), that will set up both Probs and Weights array in MachineBasicBlock, MBPI will still be using the Weights array, everything should just work, right?

I may have missed some obvious things :)

Cheers,
Manman

congh added a comment.Nov 9 2015, 3:11 PM

It seems that we can submit the changes other than SelectionDAGBuilder.h, SelectionDAGBuilder.cpp and SelectionDAGISel.cpp, separately. Correct me if I am wrong.

Thanks for working on this!
Manman

Changes under lib/Target/ should be checked in with SelectionDAG* as they are all doing instruction lowering. Other changes are for them so I think we need to put them together.

I am kind of confused. It seems to me that this change only sets up Probs array in MachineBasicBlock, but it does not start using it. When looking for getProbabilityIterator or getSuccProbability, I can't find any usage.

SelectionDAGBuilder won't use probability but only assigns correct weight/probability to new CFG.

If we commit the change in lib/Target/X86/X86FrameLowering.cpp, we will be calling the new interface addSuccor(..., BranchProb), that will set up both Probs and Weights array in MachineBasicBlock, MBPI will still be using the Weights array, everything should just work, right?

Yes. BP can be reinterpreted as weight. As we still need weights to get probabilities, at this moment we have to always maintain weight list.

I may have missed some obvious things :)

The basic idea of this patch is, we update interfaces in MBB so that SelectionDAG can use them to update probability/weight list (the former is disabled temporarily) using the new interface. Hopefully this can reduce your confusion:)

Cheers,
Manman

congh added inline comments.Nov 9 2015, 3:23 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1624 ↗(On Diff #39293)

I think it should have similar name as NewTrueProb like NewTrueProb2 so that it is obvious that we are updating two related branches's probabilities in parallel.

1632 ↗(On Diff #39293)

{A/2, B} may exceed 1 so this may not be applicable.

1842 ↗(On Diff #39293)

No. CB.TrueProb is precalculated in this file, but in some cases it can be zero, which means it is not calculated at all. In this case we use its original value from BB.

1847 ↗(On Diff #39293)

You can search normalizeSuccProbs and will find three uses in this file.

congh updated this revision to Diff 40069.Nov 12 2015, 10:50 AM
congh edited edge metadata.

Update the patch according the comments.

davidxl added inline comments.Nov 14 2015, 4:20 PM
lib/CodeGen/MachineBasicBlock.cpp
551 ↗(On Diff #40069)

Can you refactor this function let it call the iterator based removeSuccessor (in a different patch)?

587 ↗(On Diff #40069)

Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?

Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.

635 ↗(On Diff #40069)

The refactored removeSuccessor can call an internal function

removeSuccessor(succ_iterator OldI, succ_iterator NewI) -- when NewI is end(), do not update NewI's weight/prob.

replaceSuccessor can then call this method as well.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1632 ↗(On Diff #40069)

I think it will be very useful if you can introduce an interface like this (a public static member of BranchProbability) f0r the most common case:

void getNormalizedProbabilities(BranchProbability& TrueProb, BranchProbability& FalseBB);

With this interface, the low level manipulation of numerator and denominator can be eliminated and the code becomes more readable.

Note that one of the benefits of using BP interface is it helps reasoning in updating -- so we should definitely avoid doing the low level synthesis above.

congh added inline comments.Nov 16 2015, 12:40 PM
lib/CodeGen/MachineBasicBlock.cpp
551 ↗(On Diff #40069)

OK. I will let this function call iterator based removeSuccessor().

587 ↗(On Diff #40069)

Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?

I think the assertion doesn't hurt and can help us capture exceptions. What is the advantage of using an end iterator? You mean to eliminate some branches?

Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.

This should not happen: we should always guarantee that the weight list is either empty or has the same size of the successor list.

635 ↗(On Diff #40069)

Do you mean replaceSuccessor? If New is not added to the successor list, how to reference it with an iterator?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1632 ↗(On Diff #40069)

OK, this sounds good to me. In this case as 2*B may exceed one, we can obtain A/(1+B) and 2B/(1+B) by calling getNormalizedProbabilities(A/2, B). I will update the patch accordingly.

davidxl added inline comments.Nov 17 2015, 2:11 PM
lib/CodeGen/MachineBasicBlock.cpp
587 ↗(On Diff #40069)

What I mean is that with the relaxed check (temporary), you can remove the #if 0 ... #endif hack here.

635 ↗(On Diff #40069)

I mean make iterator based removeSuccessor interface more general to also take an optional NewI iterator. If NewI is Successors.end(), removeSuccessor behaves the same as the current implementation. If it is a value iterator, removeSuccessor can also be used by replaceSuccessor.

congh marked 2 inline comments as done.Nov 17 2015, 3:37 PM
congh added inline comments.
lib/CodeGen/MachineBasicBlock.cpp
587 ↗(On Diff #40069)

I am afraid that this could not remove #if 0 ... #endif as the root cause is that probability list and successor list may not be synchronized and here we are always removing a successor using an iterator to successor list.

635 ↗(On Diff #40069)

I think it is difficult to let replaceSuccessor() call removeSuccessor(). We have two cases:

  1. New is not a successor of MBB. In this case we replace Old with New without removing any successor.
  1. New is a successor of MBB. In this case we need to update the weight/probability of New. It seems the current code is more efficient: if we call removeSuccessor() to remove Old, we need to find the weight/probability of it before removing it. We don't want to search the weight/probability twice.
congh updated this revision to Diff 40445.Nov 17 2015, 3:37 PM

Update the patch according to David' comments.

congh updated this revision to Diff 40447.Nov 17 2015, 3:39 PM

Fix a bug.

congh updated this revision to Diff 40452.Nov 17 2015, 4:29 PM

Update the patch according to David's comment.

Thanks for working on this!

This time, I managed to look through all the changes. The math looks right!

include/llvm/Support/BranchProbability.h
95 ↗(On Diff #40069)

If this change is not required for replacing weight-based with probability-based, please commit it separately.

144 ↗(On Diff #40069)

Please commit this separately.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1314 ↗(On Diff #40447)

Do we need to normalize the probs after calling addSuccessorWithProb?

2049 ↗(On Diff #40447)

Do we need to normalize the probs after calling addSuccessorWithProb?

8166 ↗(On Diff #40447)

Should we call addSuccessorWithoutProb when BPI does not exist?

8352 ↗(On Diff #40447)

Will this exceed 1? Have to admit that I didn't really look into this.

congh marked 3 inline comments as done.Nov 17 2015, 5:54 PM

Thanks for working on this!

This time, I managed to look through all the changes. The math looks right!

Thank you for the review!

For the safety sake, I am wondering if we should normalize successors' probabilities for all MBB in the CFG after ISel is done. Then we can safely remove those individual normalizations in SelectionDAGBuilder.cpp.

include/llvm/Support/BranchProbability.h
96 ↗(On Diff #40452)

OK. Done.

144 ↗(On Diff #40452)

Done.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1314 ↗(On Diff #40452)

I think yes, here we need to normalize probabilities. I checked the function that calculates them and found we cannot guarantee the sum of them is 1. Thanks for pointing it out!

2049 ↗(On Diff #40452)

I think the answer is yes again here. I am wondering if we should always do it as long as we are uncertain whether the sum of successors's probabilities is one. This of cause needs potential redundant calculations but it will be safer.

8166 ↗(On Diff #40452)

addSuccessorWithProb() will take care of it, in which addSuccessorWithoutProb() is called if BPI is not available.

8352 ↗(On Diff #40452)

It should not exceed 1: LastLeft->Prob or FurstRight->Prob is the probability of a case statement and W.DefaultProb is the probability of the default statement.

congh updated this revision to Diff 40466.Nov 17 2015, 6:06 PM
congh marked 3 inline comments as done.

Update the patch according to Manman's comment.

congh added a comment.Nov 17 2015, 6:23 PM

I have separated some small patches from this one as r253417, r253421, r253425, and r253426.

davidxl added inline comments.Nov 17 2015, 6:28 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1312 ↗(On Diff #40466)

How can we make sure it is not a bug in the producer? With Probability based interface, we have a chance to enforce the producer to behave.

1518 ↗(On Diff #40466)

Why? Zero probability should be allowed.

2048 ↗(On Diff #40466)

Why can't the producer guarantee the sum is one ?-- in theory, the caller should do the normalization based on the update logic there . Blindly do normalization here may hide bugs. It is not possible for the consumer to decide if normalization is expected or there is a bug in producer.

congh updated this revision to Diff 40533.Nov 18 2015, 10:42 AM

Update the patch by allowing zero probabilities.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1312 ↗(On Diff #40466)

Agree.

1518 ↗(On Diff #40466)

OK, I think I forget to update this part.

2048 ↗(On Diff #40466)

I think bug checking is a point here. In some cases probabilities calculation is complex, and may be done in several places, and that is when validating if the sum of successors' probabilities is one is difficult. But I agree with you. Let's find a way to validate the probabilities in the future.

davidxl added inline comments.Nov 20 2015, 4:44 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1312 ↗(On Diff #40533)

Let's add a TODO here for a follow up. In follow up, find a way to assert the violations and examine the root cause

congh added inline comments.Nov 20 2015, 4:54 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1312 ↗(On Diff #40533)

OK. But why here? It is a specific place and the follow up is fixing a more general problem. After the whole patch is checked in, I will work on how to validate the successors' probabilities.

It this patch ready? I will update the test cases.

congh updated this revision to Diff 40952.Nov 23 2015, 11:15 AM

Update the patch with test cases fixes.

There is one more big change in this update: BranchProbability is now constructed as an unknown one by default (it was zero before). This forces users to initialize it before using it. In SelectionDAGBuilder, before adding a successor we check if the provided probability is unknown or not: if yes, we get the probability from BPI. Previously it checks zero probability and this is incorrect as we allow zero probabilities now.

The test case change look sane -- the weight ratio do not change before and after (and after Weight based interface is deprecated, the check-weight will become check-prob).

the only test that needs double check is the powerPC one -- is the change expected?

The test case change look sane -- the weight ratio do not change before and after (and after Weight based interface is deprecated, the check-weight will become check-prob).

the only test that needs double check is the powerPC one -- is the change expected?

The change is from block placement. This is actually a previous bug. Note the change in lib/Target/PowerPC/PPCISelLowering.cpp: 0 and 1 are added as weights but 0 is later transformed into 16. Instead we use a very small and large probabilities to replace them. That is why that test case is updated.

Ok. This change looks good to me. Please watch out for buildbot status closely. We also need to move up quickly to get the final stage ready so that some of the hacks used can be removed.

This revision was automatically updated to reflect the committed changes.