This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Fix computation of function size subject to XRay threshold
ClosedPublic

Authored by rSerge on Jun 8 2017, 4:20 AM.

Details

Summary

Currently XRay compares its threshold against Function::size() . However, Function::size() returns the number of basic blocks (as I understand, such as cycle bodies, if/else bodies, switch-case bodies, etc.), rather than the number of instructions.

The name of the parameter -fxray-instruction-threshold=N, as well as XRay documentation at http://llvm.org/docs/XRay.html , suggests that instructions should be counted, rather than the number of basic blocks.

I see two options:

  1. Count the number of MachineInstr`s in MachineFunction : this gives better estimate for the number of assembly instructions on the target. So a user can check in disassembly that the threshold works more or less correctly.
  2. Count the number of Instruction`s in a Function : AFAIK, this gives correct number of IR instructions, which the user can check in IR listing. However, this number may be far (several times for small functions) from the number of assembly instructions finally emitted.

Option 1 is implemented in this patch because I think that having the closer estimate for the number of assembly instructions emitted is more important than to have a clear definition of the metric.

Diff Detail

Event Timeline

rSerge created this revision.Jun 8 2017, 4:20 AM
rSerge added a subscriber: llvm-commits.
dberris accepted this revision.Jun 8 2017, 3:49 PM

Thanks @rSerge -- just one suggestion for readability.

lib/CodeGen/XRayInstrumentation.cpp
146–148

Could this be cleaner as a range-based for-loop?

int64_t MICount = 0;
for (const auto& MBB : MF)
  MICount += MBB.size();
This revision is now accepted and ready to land.Jun 8 2017, 3:49 PM
rSerge added inline comments.Jun 9 2017, 5:46 AM
lib/CodeGen/XRayInstrumentation.cpp
146–148

Sure, thanks, changing it.

rSerge closed this revision.Jun 9 2017, 6:23 AM
rSerge added a comment.EditedJun 9 2017, 6:27 AM

Reached mainline in https://reviews.llvm.org/rL305072 . (with Dean's comment implemented)