Page MenuHomePhabricator

[CFGPrinter] Display branch weight on the edges
ClosedPublic

Authored by anemet on Sep 1 2016, 10:22 AM.

Details

Summary

This is pretty useful especially in connection with
BFI's -view-block-freq-propagation-dags. It helped me to track down the
bug that is being fixed in D24118.

While -view-block-freq-propagation-dags displays the high-level
information with static heuristics included (and block frequencies), the
new thing only shows the raw weight as presented by PGO without any of
the static estimates. This helps to distinguished what has been
measured vs. estimated.

For the sample loop in D24118, -view-block-freq-propagation-dags=integer
looks like this:

https://reviews.llvm.org/F2381352

While with -view-cfg-only you can see the underlying branch weights:

https://reviews.llvm.org/F2381349

Diff Detail

Repository
rL LLVM

Event Timeline

anemet updated this revision to Diff 70026.Sep 1 2016, 10:22 AM
anemet retitled this revision from to [CFGPrinter] Display branch weight on the edges.
anemet updated this object.
anemet added reviewers: davidxl, dexonsmith, bogner.
anemet added a subscriber: llvm-commits.
davidxl edited edge metadata.Sep 1 2016, 11:48 AM

The problem I see is that people may confuse 'weight' with actual profile count, while in-reality it may not be the case (due to scaling). Perhaps add '(W)' after the number?

include/llvm/Analysis/CFGPrinter.h
135 ↗(On Diff #70026)

A better way to filter:

MDString *MDName = cast<MDString>(WeightsNode>getOperand(0));

if (MDName->getString() != "branch_weights")
  return "" ;
anemet marked an inline comment as done.Sep 1 2016, 4:44 PM

The problem I see is that people may confuse 'weight' with actual profile count, while in-reality it may not be the case (due to scaling). Perhaps add '(W)' after the number?

Makes sense. I went with 'W:', prepended, e.g.:

https://reviews.llvm.org/F2392296

The W after made me think of electric power ;)

anemet updated this revision to Diff 70110.Sep 1 2016, 5:14 PM
anemet edited edge metadata.

Addressed David's comments. I still kept the check of the operand number
around to make sure we don't index out of the MD node.

davidxl accepted this revision.Sep 1 2016, 5:16 PM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Sep 1 2016, 5:16 PM
This revision was automatically updated to reflect the committed changes.