Page MenuHomePhabricator

[Profile] SelectInst instrumentation Support in IR-PGO
ClosedPublic

Authored by davidxl on Aug 19 2016, 2:57 PM.

Details

Summary

Select instructions are currently ignored by PGO leading to missing profile data which is useful in guiding the select instruction lowering. This patch implements this missing feature for IRPGO.

A new counter increment intrinsic is introduced (extended from existing increment) that also takes a step argument. It is used to track the number times the true value of a select instruction is taken. The FalseValue count is derived from TrueValue count and the parent BB's profile count.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 68731.Aug 19 2016, 2:57 PM
davidxl retitled this revision from to [Profile] SelectInst instrumentation Support in IR-PGO.
davidxl updated this object.
davidxl added reviewers: xur, vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 19 2016, 5:58 PM

This is an interesting patch! I'm excited to see how the new information will be used.

I have two notes about documentation: 1) this patch needs to update docs/LangRef.rst, and 2) docs/BranchWeightMetadata.rst claims that SelectInst is not allowed to have branch_weight metadata -- so either the docs need fixing or the Verifier needs fixing (or both!). I can help with that if you like.

lib/Transforms/Instrumentation/InstrProfiling.cpp
240

Why not pass the IRBuilder in to avoid creating the DefaultStep until it's needed?

lib/Transforms/Instrumentation/PGOInstrumentation.cpp
183

override?

184

'size' is an odd name for a method in an InstVisitor. How about 'getNumSelectInsts'?

978

My impression is that this could be better structured / easier to read as two different InstVisitors. Both visitors would maintain less state than the current SelectInstVisitor, and their constructors should obviate the need SetCounterIndex and SetCounterUse.

test/Transforms/PGOProfile/select1.ll
3

Can you check that the increment-step instruction disappears with -pgo-instr-select=false?

18

'i64 [[STEP]]' instead of 'i64 %0'?

vsk added a comment.Aug 19 2016, 6:09 PM

(Instruction::extractProfTotalWeight seems OK with branch weight metadata on selects, but at a glance I can't tell whether or not SimplifyCFG uses that information. The verifier doesn't check this metadata for validity..)

davidxl marked 4 inline comments as done.Aug 22 2016, 10:55 AM
davidxl added inline comments.
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
183

It is not supposed to be virtual.

978

The instrumentation and use need to traverse IR in exactly the same way. Even though splitting it into two visitors does not change that, I'd like to keep them using the same class to show this strong tie.

davidxl updated this revision to Diff 68884.Aug 22 2016, 10:56 AM
davidxl edited edge metadata.

Addressed review feedbacks.

davidxl updated this revision to Diff 69795.Aug 30 2016, 8:58 PM

Addressed review feedbacks.

davidxl updated this revision to Diff 71682.Sep 16 2016, 11:47 AM

Addressed review comments and added LangRef documentation.

This revision was automatically updated to reflect the committed changes.
sanjoy added a subscriber: sanjoy.Sep 20 2016, 12:09 AM

Random drop by comment inline.

llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp
228 ↗(On Diff #71758)

One out of context comment here -- can't this logic be part of dyn_cast via the classof you addd to InstrProfIncrementInstStep?