[PATCH] D19435: [LowerExpectIntrinsic] pin default likely/unlikely weights to min/max values

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:42
@@ -43,1 +41,3 @@
+    "unlikely-branch-weight", cl::Hidden, cl::init(0),
+    cl::desc("Weight of the branch unlikely to be taken (default = 0)"));
 
----------------
spatel wrote:
> davidxl wrote:
> > I suggest not using 0 weight which BFI can not handle well.  1 should be better.
> I thought this over, and I really don't want to compromise on either value here. The meaning of builtin_expect() is clear, and we should honor that programmer directive. Picking semi-random values from the air can only lead to unseen problems down the road.
> 
> Can you explain why/where BFI has a problem with a '0' weight? If it's a simple bug, I will try to fix that before this patch.
I am not sure what problem you can foresee down the road with 99.95% probability proposed.  2000 is actually not something coming out of thin air -- GCC also uses this -- not that it is perfect, but I would guess lots of apps are tuned based on that setting. 

The BFI with 0 weight problem is not a simple bug -- it is due to the limitation of the propagation algorithm. See BlockFrequencyInfoImplBase::adddToDist.

Due to that, when computing BFI, 0 weight is changed to 1 anyway, so it might be better to make it an explicit 1.


http://reviews.llvm.org/D19435