[PATCH] D74809: [MBP][X86] Include static prof data when collecting loop BBs

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2516
+  if (F->getFunction().hasProfileData() || ForceLoopColdBlock ||
+      L.hasStaticProfInfo()) {
     BlockFrequency LoopFreq(0);
----------------
void wrote:
> davidxl wrote:
> > The check here seems too weak.  One branch in the loop has user annotation does not mean other branch probablity data can be trusted.  More sophisticated analysis is needed.
> Do you mean that a builtin_expect shouldn't be considered when using profile data?
No, that is what I meant.

When profile data is available, builtin_expect will be ignored -- the prof meta data is overridden by profile data reader.

What I meant is that builtin_expect has very  vague meaning -- some user uses it to indicate branch bias which can be weak, or strong.  LLVM's implementation treat it as a very strong bias (2000:1 ratio) -- so it will likely to exclude too many blocks from the block set when used in this context -- leading to code size increase or performance regression.

In other words, PGO is the way to go :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74809