[PATCH] D14973: Replace all weight-based interfaces in MBB with probability-based interfaces, and update all uses of old interfaces.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:439
@@ +438,3 @@
+  /// Probabilities list. The default probability is set as unknown. We don't
+  /// allow mixing known and unknown probabilities in successor list. When all
+  /// successors have unknown probabilities, we return 1 / N as the probability
----------------
We don't ... --> Mixing ... is not allowed.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:440
@@ +439,3 @@
+  /// allow mixing known and unknown probabilities in successor list. When all
+  /// successors have unknown probabilities, we return 1 / N as the probability
+  /// for each successor, where N is the number of successors.
----------------
We return ... ---> 1/N is returned ...

================
Comment at: include/llvm/CodeGen/MachineBranchProbabilityInfo.h:62
@@ +61,3 @@
+
+  // Same thing, but using a const_succ_iterator from Src. This is faster when
+  // the iterator is already available.
----------------
Same as above, 

================
Comment at: lib/CodeGen/BranchFolding.cpp:1107
@@ -1105,1 +1106,3 @@
 
+  // Scale down SumEdgeFreq to fit in a 32-bit integer.
+  int Scale = 0;
----------------
I remembered we talked about adding a new constructor interface in BranchProbablity to take 64 bit integer arguments -- or replace the 32bit version. The handling of overflow in intermediate step should be handled in BP -- otherwise each client will need to deal with the overflow and scaling which is not desirable.

================
Comment at: lib/CodeGen/IfConversion.cpp:1247
@@ +1246,3 @@
+    // Use zero probability as a place holder and we'll update it later.
+    BBI.BB->addSuccessor(CvtBBI->FalseBB, BranchProbability::getZero());
+    // Update the edge probability for both CvtBBI->FalseBB and NextBBI.
----------------
Is it cleaner to move this after NewFalseBB is computed?

After NewNext and NewFalse are computed, do

BBI.BB->addSuccessor (... , NewFalse);
BBI.BB->setSuccProbability(NewTrueBB, NewNext)

================
Comment at: lib/CodeGen/MachineBranchProbabilityInfo.cpp:31
@@ -30,18 +30,3 @@
 
-uint32_t MachineBranchProbabilityInfo::
-getSumForBlock(const MachineBasicBlock *MBB, uint32_t &Scale) const {
-  // First we compute the sum with 64-bits of precision, ensuring that cannot
-  // overflow by bounding the number of weights considered. Hopefully no one
-  // actually needs 2^32 successors.
-  assert(MBB->succ_size() < UINT32_MAX);
-  uint64_t Sum = 0;
-  Scale = 1;
-  for (MachineBasicBlock::const_succ_iterator I = MBB->succ_begin(),
-       E = MBB->succ_end(); I != E; ++I) {
-    uint32_t Weight = getEdgeWeight(MBB, I);
-    Sum += Weight;
-  }
-
-  // If the computed sum fits in 32-bits, we're done.
-  if (Sum <= UINT32_MAX)
-    return Sum;
+uint32_t MachineBranchProbabilityInfo::getEdgeWeight(
+    const MachineBasicBlock *Src,
----------------
It is not safe to keep this interface -- it gives user an impression that weight can be compared across edges with different Src BBs. Why not just change clients to use BP directly?


http://reviews.llvm.org/D14973