[PATCH] D86017: [BPI] Improve static heuristics for integer comparisons with CST

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:885
     // Otherwise -> Likely
     if (CI->isTrueWhenEqual())
       std::swap(TakenProb, UntakenProb);
----------------
xbolva00 wrote:
> ebrevnov wrote:
> > Looks like I misread this code originally. I thought the intent was to set "likely" if equal and "unlikely" if not. But as I understand now isTrueWhenEqual does completely different thing. I would suggest reverting this ASAP and rework.
> All perf inprovement is gone when only == is unlikely. So atleast the comment should be fixed.
It should be not only == is unlikely, but != is likely as well.  Doesn't this help?
What exact condition you have for which you need to set probabilities?



================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:980
+    // X != C -> Likely
+    switch (CI->getPredicate()) {
+    case CmpInst::ICMP_EQ:
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > ebrevnov wrote:
> > > Looks like we want to set "likely" for equal and "unlikely" not equal regardless of actual value of a constant, right? In that case I would suggest to just check that case before processing specific constant values. Moreover recently added code at line 882 should be checking exactly the same condition for not constant values, right?
> > I got perf regression when I used same condition from line 882.
> Looks like we want to set "likely" for equal and "unlikely" not equal regardless of actual value of a constant, right? In that case I would suggest to just check that case before processing specific constant values.
> 
> 
> Probably we should leave special cases 0, 1, -1.
> 
> For example, X <= 0   ->  Unlikely, but X >= 0   ->  Likely.
> 
> Probably we should leave special cases 0, 1, -1.
Sure, we shouldn't touch anything except EQ and NE at least in this patch. I don't see any difference how 0, 1, -1 are handled fro EQ and NE. 

> For example, X <= 0 -> Unlikely, but X >= 0 -> Likely.
That looks I bit strange to me that positive numbers is more likely than negative....but anyway this is existing heuristic we should not touch with out strong arguments.


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:980
+    // X != C -> Likely
+    switch (CI->getPredicate()) {
+    case CmpInst::ICMP_EQ:
----------------
ebrevnov wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > ebrevnov wrote:
> > > > Looks like we want to set "likely" for equal and "unlikely" not equal regardless of actual value of a constant, right? In that case I would suggest to just check that case before processing specific constant values. Moreover recently added code at line 882 should be checking exactly the same condition for not constant values, right?
> > > I got perf regression when I used same condition from line 882.
> > Looks like we want to set "likely" for equal and "unlikely" not equal regardless of actual value of a constant, right? In that case I would suggest to just check that case before processing specific constant values.
> > 
> > 
> > Probably we should leave special cases 0, 1, -1.
> > 
> > For example, X <= 0   ->  Unlikely, but X >= 0   ->  Likely.
> > 
> > Probably we should leave special cases 0, 1, -1.
> Sure, we shouldn't touch anything except EQ and NE at least in this patch. I don't see any difference how 0, 1, -1 are handled fro EQ and NE. 
> 
> > For example, X <= 0 -> Unlikely, but X >= 0 -> Likely.
> That looks I bit strange to me that positive numbers is more likely than negative....but anyway this is existing heuristic we should not touch with out strong arguments.
Let me clarify. I'm not suggesting to use the same condition as it is at line 882. Quite opposite. I think condition at line 882 is not correct in its current form. It should be fixed to handle EQ and NE cases only. If you don't get expected improvements with that let's look at what type of comparison you want to set probabilities for.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86017/new/

https://reviews.llvm.org/D86017