This is an archive of the discontinued LLVM Phabricator instance.

Move ProfileSummary to IR
ClosedPublic

Authored by eraman on May 16 2016, 5:30 PM.

Details

Summary

This splits ProfileSummary into two classes: a ProfileSummary class that has methods to convert from/to metadata and a ProfileSummaryBuilder class that computes the profiles summary which is in ProfileData. In subsequent patches, I will add an analysis pass to access the profile summary information.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 57425.May 16 2016, 5:30 PM
eraman retitled this revision from to Move ProfileSummary to IR.
eraman updated this object.
eraman added reviewers: silvas, vsk.
eraman added subscribers: davidxl, llvm-commits.
davidxl added inline comments.May 17 2016, 11:47 AM
include/llvm/IR/ProfileSummary.h
2 ↗(On Diff #57425)

Why is this not suitable to be in Support?

vsk added inline comments.May 17 2016, 11:56 AM
include/llvm/IR/ProfileSummary.h
2 ↗(On Diff #57425)

Nothing in Support uses llvm::{Metadata, LLVMContext}, so it would seem a bit out of place to me there.

lib/ProfileData/InstrProfReader.cpp
620 ↗(On Diff #57425)

nit, formatting

silvas edited edge metadata.May 17 2016, 3:40 PM

Sorry if this is a stupid question, but why do we need separate InstrProfSummary and SampleProfileSummary? Do you expect most client passes to do different things depending on which one it is?

include/llvm/IR/ProfileSummary.h
68 ↗(On Diff #57425)

No need to make this virtual. Just switch on Kind.
Then InstrProfSummary and SampleProfileSummary don't need to inherit.
As a rule of thumb I don't think you will need anything "protected".

Sorry if this is a stupid question, but why do we need separate InstrProfSummary and SampleProfileSummary? Do you expect most client passes to do different things depending on which one it is?

I believe it is useful to differentiate the source of profile in some cases. For example, sample profile has peculiarities where the number of samples at function entry is 0, but the function itself has non-zero samples. But I agree that we don't need separate ProfileSummary classes to differentiate this and it was a mistake on my part to have this class hierarchy. I thought I will first do the minimal change to migrate to IR and then fix this, but if you prefer I'll get rid of the SampleProfileSummary and InstrProfSummary classes (and have an enum for the profile source) in this patch itself.

silvas accepted this revision.May 17 2016, 4:13 PM
silvas edited edge metadata.

Sorry if this is a stupid question, but why do we need separate InstrProfSummary and SampleProfileSummary? Do you expect most client passes to do different things depending on which one it is?

I believe it is useful to differentiate the source of profile in some cases. For example, sample profile has peculiarities where the number of samples at function entry is 0, but the function itself has non-zero samples. But I agree that we don't need separate ProfileSummary classes to differentiate this and it was a mistake on my part to have this class hierarchy. I thought I will first do the minimal change to migrate to IR and then fix this, but if you prefer I'll get rid of the SampleProfileSummary and InstrProfSummary classes (and have an enum for the profile source) in this patch itself.

Feel free to do the refactoring either before or after the move to IR (unless the refactoring is needed to avoid layering issues; I don't think that is the case here). I agree that we should keep the move as mechanical as possible.

This revision is now accepted and ready to land.May 17 2016, 4:13 PM
silvas added inline comments.May 17 2016, 4:14 PM
lib/ProfileData/ProfileSummaryBuilder.cpp
28 ↗(On Diff #57425)

Nit: in a separate patch it would be nice to make this a constant array to avoid the static constructor.

eraman marked an inline comment as done.May 18 2016, 3:04 PM

Sorry if this is a stupid question, but why do we need separate InstrProfSummary and SampleProfileSummary? Do you expect most client passes to do different things depending on which one it is?

I believe it is useful to differentiate the source of profile in some cases. For example, sample profile has peculiarities where the number of samples at function entry is 0, but the function itself has non-zero samples. But I agree that we don't need separate ProfileSummary classes to differentiate this and it was a mistake on my part to have this class hierarchy. I thought I will first do the minimal change to migrate to IR and then fix this, but if you prefer I'll get rid of the SampleProfileSummary and InstrProfSummary classes (and have an enum for the profile source) in this patch itself.

Feel free to do the refactoring either before or after the move to IR (unless the refactoring is needed to avoid layering issues; I don't think that is the case here). I agree that we should keep the move as mechanical as possible.

Yes, the refactoring is not needed to avoid the layering issues. I'll get the other patch reviewed and submit this patch followed by the other one back-to-back.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/ProfileData/InstrProfWriter.cpp