[PATCH] D111750: [llvm-profgen] Allow unsymbolized profile as perf input

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:310
 
 std::string PerfReaderBase::convertPerfDataToTrace(ProfiledBinary *Binary,
                                                    StringRef PerfData) {
----------------
How about we use `PerfInputFile PerfReaderBase::convertPerfDataToTrace(ProfiledBinary *Binary, PerfInputFile &PerfInput`), and return a new PerfInput from the function?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:732-739
+inline void exitWithErrorForTraceLine(TraceStream &TraceIt) {
+  std::string Msg = TraceIt.isAtEoF()
+                        ? "Invalid raw profile!"
+                        : "Invalid raw profile at line " +
+                              Twine(TraceIt.getLineNumber()).str() + ": " +
+                              TraceIt.getCurrentLine().str();
+  exitWithError(Msg);
----------------
Can we move this a lambda instead of global function since it's only used in readSampleCounters?

If lambda grows too big, we can use member functions too. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:741
+
+void RawProfileReader::readSampleCounters(TraceStream &TraceIt,
+                                          SampleCounter &SCounters) {
----------------
Some comment showing the expected input for parsing would be helpful.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:771
+
+      if(!UseOffset) {
+        Source = Binary->virtualAddrToOffset(Source);
----------------
If we use this flag to control profile reading too, we need to update flag description. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:788-789
+  while (!TraceIt.isAtEoF()) {
+    std::shared_ptr<StringBasedCtxKey> Key =
+        std::make_shared<StringBasedCtxKey>();
+    StringRef Line = TraceIt.getCurrentLine();
----------------
The underlying object created by make_shared stays on heap, but SampleCounters.emplace creates another copy of the object? is that intentional or did I miss anything? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:797
+    }
+    Key->genHashCode();
+    auto Ret =
----------------
Would it better if hash code is lazily generated instead of requiring an explicit call? 

```
getHashCode() {
  if (HashCode == 0) 
     genHashCode()
  ...
}
```


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:807-808
+
+  if (SkipSymbolization)
+    writeRawProfile(OutputFilename);
+}
----------------
So if user specify skip-symbolization and the input is raw profile, we are just reading it in and then writing it out without doing anything? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:63
+enum PerfInputType {
+  PERFSCRIPT_UNKNOWN = 0,
+  PERFSCRIPT_INVALID = 1,
----------------
nit: all caps is usually only used for macro. For enum, it's better to follow normal variable naming, which is also the convention used in llvm 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:63-68
+  PERFSCRIPT_UNKNOWN = 0,
+  PERFSCRIPT_INVALID = 1,
+  PERFSCRIPT_LBR = 2,       // Only LBR sample
+  PERFSCRIPT_LBR_STACK = 3, // Hybrid sample including call stack and LBR stack.
+  PEFFDATA = 4,             // Raw linux perf.data.
+  RAWPROFILE = 5,           // Unsymbolized profile generated by llvm-profgen.
----------------
wenlei wrote:
> nit: all caps is usually only used for macro. For enum, it's better to follow normal variable naming, which is also the convention used in llvm 
These are not mutually exclusive types. e.g. PERFSCRIPT_UNKNOWN can be either PERFSCRIPT_LBR or PERFSCRIPT_LBR_STACK. 

Logically there're two things being represented, the format and the content. To avoid exponential combination and to have a clear separation, perhaps use format, content masks instead, or even separate them out?

We can have format = RawProfile|PerfScript|PerfData, and content = LBR|LBRStack|Aggregated.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:74-75
+  PerfInputType InputType;
+  PerfInputFile(StringRef F, PerfInputType T)
+      : InputFile(F.str()), InputType(T){};
 };
----------------
nit: it's cleaner to keep it a POD type and remove the ctor. we can use initializer list to create the object.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:39
+cl::opt<std::string>
+    RawProfileFilename("raw-profile", cl::value_desc("raw-profile"),
+                       cl::ZeroOrMore, llvm::cl::MiscFlags::CommaSeparated,
----------------
I'm not sure if raw profile is a good name. the real raw profile is the perf.data, after some processing we get perf script, and with more processing we get this "raw profile". Perhaps we need a alternative name, unsymbolized profile? 


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:102
 
+static PerfInputFile createPerfInputFile() {
+  // Parse perf events and samples
----------------
nit: createPerfInputFile -> getPerfInputFile


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111750/new/

https://reviews.llvm.org/D111750