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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:357-360
+  PerfInputFile RetFile;
+  RetFile.InputFile = PerfTraceFile;
+  RetFile.Format |= PerfFormat::PerfScript;
+  return RetFile;
----------------
wenlei wrote:
> nit: we can do `return {PerfTraceFile, PerfFormat::PerfScript, PerfContent::Unknown};`
Good to know this, thanks


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:797
+    }
+    Key->genHashCode();
+    auto Ret =
----------------
wenlei wrote:
> wlei wrote:
> > wenlei wrote:
> > > Would it better if hash code is lazily generated instead of requiring an explicit call? 
> > > 
> > > ```
> > > getHashCode() {
> > >   if (HashCode == 0) 
> > >      genHashCode()
> > >   ...
> > > }
> > > ```
> > Here to explicitly call `genHashCode` is intentional, the reason is we want to avoid making `genHashCode` a virtual function, i, e, avoid call `genHashCode `after casting to base class. so we separate like:
> > ```
> > derived class:  HashCode = genHashCode();
> > 
> > base class :  getHashCode{return HashCode;}
> > ```
> > 
> > 
> Not sure if I understand. Why do we want to avoid call genHashCode after casting to base class?
Currently we have two type of key, `StrKey` and `ProbeKey` and they derived from the base  `ContextKey`.

We have a hash map to store them, like unordered_map<ContextKey, ...>, the logic of insert is like
```
StrKey* key = ..

hashmap[key] = ...;   
```
In the hashmap,  `StrKey*/ProbeKey*` implicitly be cast to a ContextKey* class then call `ContextKey->getHashCode` , so `getHashCode` should be a virtual function which has overhead because  `StrKey` and `ProbeKey` have different genHashCode. 

So to avoid this, we can explicitly call StrKey->genHashCode() before being casting to base and store it into a variable `HashCode`

then base's getHashCode just read the HasCode, no need a virtual function.








================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:640
+/*
+   Format of unsymbolized profile:
+
----------------
wenlei wrote:
> Also comment to explain the difference between CS and non-CS profile?
comments updated


================
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:
> wlei wrote:
> > wlei wrote:
> > > wenlei wrote:
> > > > 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.
> > > Fixed, thanks!
> > Makes sense.Changed to separate them out.
> Now that format and content are separated, perhaps they no longer need to use mask within each? Sorry if I wasn't clear, mask or separate field are two ways to have separation for format and content. 
I see, thanks for the clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111750