This is an archive of the discontinued LLVM Phabricator instance.

[Doc] Document prof metadata in LangRef
ClosedPublic

Authored by tejohnson on Jun 14 2017, 12:52 PM.

Details

Summary

Points to existing documentation for branch_weights and
function_entry_count, and adds an example for VP value profile metadata.

Event Timeline

tejohnson created this revision.Jun 14 2017, 12:52 PM
davidxl added inline comments.Jun 14 2017, 1:12 PM
docs/LangRef.rst
5210

Does it attach to the call instruction?

It can be attached to SELECT instruction which should be mentioned.

5220

to record the number of times the function is called. Used with BFI information, it is also used to derive basic block profile count.

5231

memcpy -- mempy like calls (including memset, etc)

tejohnson added inline comments.Jun 14 2017, 1:22 PM
docs/LangRef.rst
5210

It attached to direct call instructions in SamplePGO mode. And according to the linked older documentation I found (http://llvm.org/docs/BranchWeightMetadata.html#indirectbrinst) it attaches to indirect calls, although I wonder if that is stale since we now have VP metadata for indirect calls?

Will add "select" to the list of instruction types here.

5220

will update.

5231

will change to "memory intrinsics such as memcpy, memmove and memset"

tejohnson updated this revision to Diff 102600.Jun 14 2017, 1:25 PM
  • Address comments
davidxl added inline comments.Jun 14 2017, 1:32 PM
docs/LangRef.rst
5210

Need to document the direct call usage. indirectbr is not for indirect call, but computed gotos. So may be expand branch into conditional branch or indirect branches? Unconditional direct branch does not need this meta data.

tejohnson marked an inline comment as done.Jun 14 2017, 1:49 PM
tejohnson updated this revision to Diff 102602.Jun 14 2017, 1:49 PM
  • Document branch_weight metadata on direct calls with SamplePGO
reames edited edge metadata.Jun 14 2017, 2:59 PM

Thank you for acting on this quickly. Comments inline, but nothing here should prevent this landing. We can iterate on the documentation once there's *something* in place.

docs/BranchWeightMetadata.rst
70

Please describe the semantics of the metadata separately from the usage. I have an alternate profiling source and need to know how the metadata is used, not just where you assume it comes from.

docs/LangRef.rst
5237

It would have been more clear to represent the types as strings in the metadata. Document what's there for the moment, but might be worth changing this.

Clarification: Is it legal for the sum of the pairs to not add up to the total? (It should be.)

Clarification: Is it *required* that the counts be exact executio counts? Or are they solely relative weightings? (Should be the later.)

Thank you for acting on this quickly. Comments inline, but nothing here should prevent this landing. We can iterate on the documentation once there's *something* in place.

Sure. Can you or David LGTM the patch if this looks good for now?

docs/BranchWeightMetadata.rst
70

Right now it is only used in SamplePGO mode (used to detect hot calls since profile counts on the function entry etc may not be accurate in sampling mode). I've updated the wording to indicate this.

docs/LangRef.rst
5237

It would have been more clear to represent the types as strings in the metadata. Document what's there for the moment, but might be worth changing this.

David may be able to give some context on why it is an int and not a string. But as a user I agree a string would be clearer.

Clarification: Is it legal for the sum of the pairs to not add up to the total? (It should be.)

Yes. In fact, we only profile the top N (N is configurable, default is 8), so the value counts often don't add up to the total. I have clarified this.

Clarification: Is it *required* that the counts be exact execution counts? Or are they solely relative weightings (Should be the later.)

Relative weightings should work fine. In fact, the total may not be equal to the BB execution count in a multi-threaded profile collection environment since counter updates are non-atomic.

tejohnson updated this revision to Diff 102671.Jun 15 2017, 7:51 AM
  • Address comments
davidxl accepted this revision.Jun 15 2017, 8:50 AM

lgtm

This revision is now accepted and ready to land.Jun 15 2017, 8:50 AM
This revision was automatically updated to reflect the committed changes.