This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Delay calculation of Cycles per Resources, separate the cycles and resource quantities.
ClosedPublic

Authored by mattd on Sep 10 2018, 6:12 PM.

Details

Summary

This patch removes the storing of accumulated floating point data
within the llvm-mca library.

This patch splits-up the two quantities: cycles and number of resource units.
By splitting-up these two quantities, we delay the calculation of "cycles per resource unit"
until that value is read, reducing the chance of accumulating floating point error.

I considered using the APFloat, but after measuring performance, for a large (many iteration)
sample, I decided to go with this faster solution.

Diff Detail

Event Timeline

mattd created this revision.Sep 10 2018, 6:12 PM

Hi Matt,

tools/llvm-mca/include/HWEventListener.h
60–64

I find this comment very hard to read. Maybe it is just my English, however it doesn't what exactly do you mean by reducing floating point error.
The "floating point error" is a loss of precision.

I think you could simply write that class ResourceCycles defines fractions of cycles. Objects are essentially divisions, described by a pair of unsigned. The numerator is a number of cycles, while the denominator is a number of processor resource units.

You can then say that this class is currently used by the ResourcePressureView to compute average resource cycles per instruction/iteration.

98

Do we actually need this operator? If it is not used, then please remove it.
we can always add it in future if we think that we actually need it.

tools/llvm-mca/include/HardwareUnits/ResourceManager.h
19

This is not nice.
There shouldn't be any reference to HWEventListener from any hardware unit.
This is equivalent to a layering violation to me. Events and event listeners are a concept only known by stages and pipelines.

Either you move this into Instruction.h, or Support.h.

But definitely it should never be in HWEventListener.h

tools/llvm-mca/include/HardwareUnits/Scheduler.h
18

Same. This is not okay.

I noticed that you forgot to add llvm-commits to the subscribers list.
Adding it now.

mattd updated this revision to Diff 164913.Sep 11 2018, 9:47 AM
mattd marked 3 inline comments as done.
mattd retitled this revision from [llvm-mca] Delay calculation of Cycles per Resources to reduce fp error. to [llvm-mca] Delay calculation of Cycles per Resources, separate the cycles and resource quantities..
mattd edited the summary of this revision. (Show Details)
  • Updated the ResourceCycles class description.
  • Moved that class into Support.h.
  • Removed an unused + operator.
andreadb accepted this revision.Sep 11 2018, 9:56 AM

LGTM.

tools/llvm-mca/include/HWEventListener.h
104–107

You can probably add a using to give a name to type std::pair<ResourceRef, ResourceCycles>. I saw that it is used a few times (not just in this class). We should consider adding it if that makes code a bit more readable.

tools/llvm-mca/include/Support.h
29 ↗(On Diff #164913)

per instruction/iteration.

This revision is now accepted and ready to land.Sep 11 2018, 9:56 AM
This revision was automatically updated to reflect the committed changes.
mattd marked an inline comment as done.