[PATCH] D123271: [llvm-profgen] Filter out invalid LBR ranges.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:304
 
-    if (isCallState(State)) {
+    if (isInvalidState(State)) {
+      // Skip unwinding the rest of LBR trace. Reset the stack and update the
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > nit: put this towards the end of the if-else sequence as it's supposed to be a corner case. 
> > > 
> > > ```
> > > } else if (isValidState(State)) {
> > >     // Unwind branches
> > >     ...
> > >    unwindBranch(State);
> > > }
> > > else {
> > >     // Invalid state
> > > }
> > > 
> > > ```
> > That'd require to change `isCallState` ... to. check for the invalid flag. It only checks if the current ip corresponds to a call inst.
> Yes, I think we should change all isXXState to check validness first anyways. An invalid state isn't any of the valid states.
Sounds good.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:305-308
+      // Skip unwinding the rest of LBR trace. Reset the stack and update the
+      // state so that the rest of the trace can still be processed in a
+      // context-insensitive way, i.e, all ranges will be counted towards
+      // the root context.
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > > all ranges will be counted towards the root context.
> > > 
> > > When we skip unwinding, we still have the context carried with probe/dwarf, so it's not always going to be base context, right? 
> > Yes, ranges themselves carry inline contexts. The wording may be confusing. I'm changing it to "the rest of the trace can still be processed as if they do not have a calling context."
> they do not have a calling context. -> they do not have stack sample?
done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123271