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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:286-287
 
+  // Skip unwinding the rest of LBR trace when a bogus range is seen.
+  bool SkipUnwinding = false;
+
----------------
Is it critical to skip unwinding instead of ignore the entire record for invalid range? If not, perhaps it's better to keep it simple and just ignore such record. I imagine such invalid ranges are rare. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:300-301
       // break into smaller sub section each with its own calling context.
-      unwindLinear(State, Repeat);
+      if (!unwindLinear(State, Repeat))
+        SkipUnwinding = true;
     }
----------------
Theoretically any kind of unwind can discover bad state, so this is not unique to unwindLinear. I suggest we have an invalid state flag for UnwindState, which gets checked for all isXXState (may also added isValidState). This way we also keep the API for unwindXX consistent, and also avoid special checking after unwindLinear. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1187
       "profile and binary mismatch.";
+  const char *BogusRangeMsg = "Range start is above range end.";
 
----------------
nit: above->after


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1202
         !Binary->offsetIsTransfer(EndOffset)) {
-      InstNotBoundary++;
+      InstNotBoundary += I.second;
       WarnInvalidRange(StartOffset, EndOffset, EndNotBoundaryMsg);
----------------
I think the choice to use unique ranges was intentional. What motivated this change?  


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1218-1221
+    if (StartOffset > EndOffset) {
+      BogusRange += I.second;
+      WarnInvalidRange(StartOffset, EndOffset, BogusRangeMsg);
+    }
----------------
Such invalid ranges doesn't seem to be recorded anyways, how do we get those ranges? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1230
                      "of profiled ranges do cross function boundaries.");
+  emitWarningSummary(BogusRange, TotalRangeNum, "of profiled ranges is bogus.");
 }
----------------
It is unclear to user what "bogus" means. And one could argue that ranges crossing function boundaries (one above) is also bogus. Suggest to be specific about what this is so user knows what this means, e.g. `... of profiled ranges have range start after range end`.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:297
   void advanceLBR() { LBRIndex++; }
-  ProfiledFrame *getParentFrame() { return CurrentLeafFrame->Parent; }
+  ProfiledFrame *getParentFrame() {
+    return CurrentLeafFrame == &DummyTrieRoot ? CurrentLeafFrame
----------------
What motivated this change? Conceptually parent frame of dummy root should indeed be nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123271