[PATCH] D30631: [BPI] Use metadata info before any other heuristics

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:308-311
+      auto MetadataProb = BranchProbability::getBranchProbability(
+          Weights[i], static_cast<uint32_t>(WeightSum));
+      if (UnreachableProb < MetadataProb)
+        return false;
----------------
skatkov wrote:
> chandlerc wrote:
> > I feel like it would be nicer to just adjust the weight downward such that the probability is essentially the minimum of the two sources of information. That way we don't lose the metadata's weights for the different successors that *don't* go to unreachable.
> > 
> > Consider the test case (in pseudo C code):
> > 
> >   for (...) {
> >     switch (cond) {
> >       default:
> >         unreachable
> >       case 2: // HOT
> >         // something tiny
> >         continue;
> >       case 3: // COLD
> >         // huge pile of ugly code
> >         continue;
> >       }
> >     }
> >   }
> > 
> > If, for whatever reason, we end up with one sample in the metadata going to unreachable, we'll completely loose the metadata that distinguishes between hot and cold here.
> > 
> > Does that make sense?
> It is possible, however I try to follow simpler logic here. So the main question if we do not trust that metadata represents the value for unreachable edge correctly (we fix it by the weight downward) why we trust that data for hot/cold edges is valid and continue using it?
> 
> However if you still insist on that I would propose I will create a follow up patch implementing this approach and leave this patch as is. Is it ok for you?
> 
> 
It's not that I don't trust the metadata edge, it's about what is the strongest signal to the optimizer.

When we have an unreachable, we don't need to wonder about what the metadata says because we have a control flow reason to know we shouldn't optimize that path. It isn't that the metadata is definitely wrong or bad, it is that the CFG analysis is definitely sufficient.

So we shouldn't throw out the metadata for the reachable successors IMO.

I think it would be most clear to do it in this patch. Is there a problem with doing that?


https://reviews.llvm.org/D30631