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

[Rong Xu via Phabricator via llvm-commits]

xur marked 3 inline comments as done.
xur added a comment.

Thanks to Wenlei for the comments!



================
Comment at: llvm/include/llvm/CodeGen/FSAFDODiscriminator.h:1
+//===----- FSAFDODiscriminator.h: FS Discriminator Support --0---*- c++ -*-===//
+//
----------------
wenlei wrote:
> 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? 
Yes. This is true. The IR level FSDiscriminator could a revised version of current AddDiscriminator pass. I'll follow your suggestion and do the renaming.


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:1
+//===- llvm/Support/FSAFDODiscriminator.h -----------------------*- C++ -*-===//
+//
----------------
wenlei wrote:
> 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. 
This is doable. Let me try.


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:17
+
+#include "llvm/Support/ErrorHandling.h"
+
----------------
wenlei wrote:
> Is this needed?
This was needed by other functions that were split from this patch. Thanks for catching this. I'll remove it.


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:36
+static inline unsigned getN1Bits(int N) {
+  if (N >= 31)
+    return 0xFFFFFFFF;
----------------
wenlei wrote:
> Is `N>31` actually expected? could assert?  
should not be greater than 31. Assert is good idea.


================
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());
----------------
wenlei wrote:
> 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. 
For the case of empty name and linkagename, they do return the same hash and may result in a conflict. But I don't thing we can do much here - it just means we cannot use call stack to differentiate.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:586
           if (const DILocation *DIL = I.getDebugLoc()) {
             auto NewDIL = DIL->cloneByMultiplyingDuplicationFactor(ULO.Count);
             if (NewDIL)
----------------
wenlei wrote:
> 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. 
I was thinking the same. But decided to do this way to favor the compile time. We might want to deprecate multiply factor later, so all this might be just a temporary thing.
I will add an assert in cloneByMultiplyingDuplicationFactor 


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

https://reviews.llvm.org/D102246