[PATCH] D13963: Create a new interface addSuccessorWithoutWeight(MBB*) in MBB to add successors when optimization is disabled.

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:511
@@ -516,1 +510,3 @@
+  // diabled optimization) or has the same size as successor list.
+  if (!Weights.empty() || Successors.empty())
     Weights.push_back(Weight);
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > I find it more readable to say
> > > 
> > > if (! (Weights.empty() && !Successors.empty())) {
> > >    ...
> > > }
> > > 
> > > Since this interface is called only when optimization enabled, so 
> > >  (Weights.empty () && Successors.empty()) == false should be asserted -- basically the above if condition is always  true?
> > > I find it more readable to say
> > > 
> > > if (! (Weights.empty() && !Successors.empty())) {
> > > 
> > > ...
> > > }
> > 
> > Yes.
> > 
> > > 
> > > Since this interface is called only when optimization enabled, so 
> > > (Weights.empty () && Successors.empty()) == false should be asserted -- basically the above if condition is always true?
> > 
> > This is not true. When optimization is disabled, this interface can also be called, even the added weights are never used. That is to say, we cannot not guarantee empty weight list when optimization is disabled.
> > 
> > 
> Basically if the addSuccessor(..) interface is used for the first successor (when opt is disabled), then the weight list will be created. Does that explain the test case result difference?
> 
> Also what happens if addSuccessor is first called, and then followed by addSuccessorWithoutWeight -- then the assert (Weights.empty()) will be triggered.
> Basically if the addSuccessor(..) interface is used for the first successor (when opt is disabled), then the weight list will be created. Does that explain the test case result difference?

The test case is run with optimization enabled, so this cannot explain the difference. The different comes from the fact if a MBB has all successors with default weight, then we have an empty weight list before this patch but with this patch we will get a weight list with default weights (0 here).

> 
> Also what happens if addSuccessor is first called, and then followed by addSuccessorWithoutWeight -- then the assert (Weights.empty()) will be triggered.

Yes, that assertion should be triggered. But it is never triggered on all test cases.




http://reviews.llvm.org/D13963