[PATCH] D11442: Provide an interface normalizeSuccWeights in MachineBasicBlock to normalize its successors' weights and use it in other places.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: test/Transforms/SampleProfile/branch.ll:41
@@ -41,2 +40,3 @@
+; CHECK: edge entry -> if.end probability is 1 / 2 = 50%
 
 if.end:                                           ; preds = %entry
----------------
congh wrote:
> davidxl wrote:
> > Since BFI always ignores zero weight which leads to inconsistency (as BPI did not ignore zero weight before this fix, only BFI does it).
> > 
> > If the test case really intend to test a highly biased branch here, I think the test case should be modified to not use zero weight after this fix (as it now will be completely ignored).  Changing the the results to 50% may violate what is intended by the test.
> I did an investigation on this. The weight 0 and 1 are not indicated in this test cased but are read from a sample profile file, in which most lines have zero weights except two which are large weights. Where does 1 come from? When a zero weight is read from sample profile, it is converted to 1 (see include/llvm/ProfileData/SampleProf.h:175). In the comment, it says that the weight is set to 0 if the profile data is missing. If the profile data exist and the number of samples is zero, it will be converted to 1 to differentiate those two different cases. However, this will lead to a possible result that two edges from the same block have weight 0 and 1 (0 for missing profile and 1 for 0 sample): now how to interpret them? I think here it is a good idea to convert 0 to 1 in BPI for this case. For the original test case the probability 0 and 100% are quite wrong, but 50% and 50% as in the updated test case is more reasonable.
In AutoFDO, lines with instructions generated but without any samples are annotated with 0 counts -- they actually should not be converted to 1 as 0 counts are strong signals of coldness.

One the other hand, lines that do not have any instructions associated (due to CSE, PRE, IVOPT etc) do not have any counts either, but they should not be marked with 0 counts and should be specially handled.  It seems current implementation tries to overload zero weight again to represent this state which is wrong because zero weight does not do what it thinks it does.

At this point, I think this test does not matters that much (Dehao and Diego are working to overhaul SampleFDO support anyway) -- so I am ok with this change.

Duncan, WDYT?


http://reviews.llvm.org/D11442