Page MenuHomePhabricator

[InstrProf][NFC] Refactor Profile kind into a bitset enum.
ClosedPublic

Authored by snehasish on Dec 8 2021, 1:32 PM.

Details

Summary

This change refactors the ProfileKind enum into a bitset enum to
represent the different attributes a profile can have. This change
simplifies the logic in the instrprof writer when multiple profiles are
merged together. In the future we plan on introducing a new memory
profile section which will extend the enum by one additional entry.
Without this change when accounting for memory profiles will have to be
maintained separately and will make the logic more complex.

Diff Detail

Event Timeline

snehasish created this revision.Dec 8 2021, 1:32 PM
snehasish requested review of this revision.Dec 8 2021, 1:32 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 8 2021, 1:32 PM
snehasish updated this revision to Diff 392927.Dec 8 2021, 1:48 PM

Cleanup for review -

  • Added some more descriptive comments.
  • Removed a couple of unintentional blank lines.
  • Removed a couple of commented lines of code.
tejohnson added inline comments.Dec 8 2021, 1:50 PM
llvm/include/llvm/ProfileData/InstrProfReader.h
94

It seems like these helper methods could still be defined using the new bitset, which would reduce some of the code churn?

snehasish updated this revision to Diff 392937.Dec 8 2021, 2:31 PM

Reintroduce the helpers while keeping the bitset.

snehasish updated this revision to Diff 392938.Dec 8 2021, 2:33 PM

Remove extra line in PGOInstrumentation.cpp.

snehasish added inline comments.Dec 8 2021, 2:41 PM
llvm/include/llvm/ProfileData/InstrProfReader.h
94

My rationale for removing the helpers was

  • to have only one way of checking the bits
  • the checks are clustered together so users would get the bitset once rather than invoking multiple methods for each check
  • extending the bitset would add now mean that adding more helpers?
  • retaining the helpers doesn't obviate the need for getProfileKind since the simplified code in mergeProfileKind directly operates on the enum value

I agree retaining the helpers reduces code churn and I've made the changes to keep them. PTAL, thanks!

tejohnson accepted this revision.Dec 8 2021, 9:48 PM

lgtm

llvm/include/llvm/ProfileData/InstrProfWriter.h
48

Was the InstrEntryBBEnabled parameter just never used? I didn't see any changes to callsites.

This revision is now accepted and ready to land.Dec 8 2021, 9:48 PM

@xur Could you please take a look with a focus on ensuring that this change retains the semantics that you expect? Previously there were different enum values which indicated CS+IR vs IR only. Now the IR bit is set for both cases (to be consistent with the version mask). I believe this was the original intent. The only change I had to make was reorder the print in the text format writer.

llvm/include/llvm/ProfileData/InstrProfWriter.h
48

I didn't find any uses in llvm and I don't see it being used in the original commit: https://git.io/JDt8k. There is a chance that it could be used by a downstream user but it's unlikely? I'll wait for @xur to chime in.

ellis added inline comments.Jan 27 2022, 10:36 AM
llvm/include/llvm/ProfileData/InstrProf.h
281–282

I've been working on a new coverage instrumentation in D116180 that I guess would need to be added this this enum. The plan was to first add function entry coverage, then basic block coverage.

snehasish added inline comments.Jan 27 2022, 10:45 AM
llvm/include/llvm/ProfileData/InstrProf.h
281–282

Yes this makes the enum composable and brings it in line with the Variant mask encoding in the indexed profile header. Let me go ahead and submit this (after rebasing and testing) so that you can rebase your changes. Thanks for the heads up!

This revision was landed with ongoing or failed builds.Jan 27 2022, 1:01 PM
This revision was automatically updated to reflect the committed changes.
mtrofin added inline comments.
llvm/include/llvm/ProfileData/InstrProfReader.h
495

This looks a lot like line 290, can it be refactored (or am I missing something?)

llvm/lib/ProfileData/InstrProfWriter.cpp
337

consider adding helper APIs for test/set?

ellis added inline comments.Jan 27 2022, 1:33 PM
llvm/include/llvm/ProfileData/InstrProf.h
284–287

Can we expand these names instead of using acronyms? It would help us remember what these flags do by being more descriptive.

Example: InstrProfKind::BB => InstrProfKind::WithFunctionEntry

snehasish added inline comments.Jan 31 2022, 2:39 PM
llvm/include/llvm/ProfileData/InstrProfReader.h
495

Since these were in a header but spread across different inherited classes of the same base I moved these to the .cpp and refactored it into a common static function in https://reviews.llvm.org/D118656. Thanks!

llvm/lib/ProfileData/InstrProfWriter.cpp
337

I'm not sure it will help much but I'll take a look in the patch to rename the enum types to more descriptive names.