[PATCH] D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/FSAFDODiscriminator.h:1
+//===----- FSAFDODiscriminator.h: FS Discriminator Support --0---*- c++ -*-===//
+//
----------------
IIUC, FSDiscriminator can be added to IR pass too, possibly with the existing AddDiscriminator IR pass plus tweak on discriminator encoding.

The current implementation makes it look like FSDiscriminator is tied to MIR pass alone.. Perhaps name the files/pass for MIR `AddMIR[FS]Discriminator` etc so it doesn't convey the message that FSDiscriminator is tied to MIR? 


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:1
+//===- llvm/Support/FSAFDODiscriminator.h -----------------------*- C++ -*-===//
+//
----------------
I wondering if it also makes sense to pull all existing discriminator encoding stuff into a single header, for consistency. Then it can be `Discriminator.h` 

encodeDiscriminator/decodeDiscriminator for today's discriminator encoding are static functions that does pure encoding and can live anywhere. 


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:17
+
+#include "llvm/Support/ErrorHandling.h"
+
----------------
Is this needed?


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:36
+static inline unsigned getN1Bits(int N) {
+  if (N >= 31)
+    return 0xFFFFFFFF;
----------------
Is `N>31` actually expected? could assert?  


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:47
+  uint64_t Ret = MD5Hash(std::to_string(DIL->getLine()));
+  Ret ^= MD5Hash(BB.getName());
+  Ret ^= MD5Hash(DIL->getScope()->getSubprogram()->getLinkageName());
----------------
xur wrote:
> hoy wrote:
> > `BB.getName()` and `getSubprogram()->getLinkageName()` could be empty. Could it be a problem?
> MD5Hash can take empty string. I did not encounter issues for this.
Empty string may not be a big/noticeable problem as it just slightly increase the chance of hash collision. I think in many places, we fall back to `getName` when `getLinkageName` is empty. 


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:586
           if (const DILocation *DIL = I.getDebugLoc()) {
             auto NewDIL = DIL->cloneByMultiplyingDuplicationFactor(ULO.Count);
             if (NewDIL)
----------------
It's probably cleaner to just bail out inside `cloneByMultiplyingDuplicationFactor` by checking `EnableFSDiscriminator`, return `this` if that flag is on. That would avoid checks for the 3 loop passes. But for compile time, there's no reason to go through the IR if we aren't going to do anything.

If we skip these from the loop passes, how about also assert in `cloneByMultiplyingDuplicationFactor` that `EnableFSDiscriminator` if off as a safe guard. 


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

https://reviews.llvm.org/D102246