[PATCH] D13745: Replace all uses of branch weights by branch probabilities on machine code level passes.

[Manman Ren via llvm-commits]

manmanren added a subscriber: manmanren.
manmanren added a comment.

I would like to see this patch being separated into smaller ones if possible. We can keep both addSuccessor(..., Weight) and addSuccessor(..., BranchProbability) for a short period of time to ease the transition.

I am mostly concerned about the changes in updating branch weights/probs during transformation (SelectionDAGBuilder, BranchFolding, IfConversion etc). If we can separate the changes, it will be much easier to review and to debug later on if we have a performance issue.


================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:121
@@ +120,3 @@
+    return IsLikely ? LikelyProb : LikelyProb.getCompl();
+  }
+
----------------
Should we remove getBranchWeightStackProtector after getBranchProbStackProtector is added?

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:427
@@ -425,3 +426,3 @@
   /// Note that duplicate Machine CFG edges are not allowed.
-  void addSuccessor(MachineBasicBlock *Succ, uint32_t Weight = 0);
+  void addSuccessor(MachineBasicBlock *Succ, BranchProbability Prob);
 
----------------
Can you comment on if the sum of probabilities for all successors is 1, after adding the successor?

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:433
@@ +432,3 @@
+  /// to keep their relative relations.
+  void addSuccessor(MachineBasicBlock *Succ);
+
----------------
davidxl wrote:
> Why do we want to have this interface?  If the client does not know what probability to use, it won't be too hard to specify 1/N either.
We seem to normalize the probabilities in this version of addSuccessor, but not the previous one. Any reason?

================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:462
@@ -461,3 +461,3 @@
     }
-    MBB.addSuccessor(SuccMBB, Weight);
+    MBB.addSuccessor(SuccMBB, BranchProbability(Prob, 100));
   } while (consumeIfPresent(MIToken::comma));
----------------

Can you add a comment for using 100? Because it is printed out as percentile?
It is kind of weird to print out as (0xbb / 0xbb = 33.0%), and to parse as (33).
Can you round-trip the printout?


http://reviews.llvm.org/D13745