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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:41
+
+#ifndef NDEBUG
+// Given a discriminator value DiscriminatorVal, return the number of buckets
----------------
I don't think these helpers are used in this patch. If so can we remove them from this patch and add it when they are actually used?


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:24
+using namespace llvm;
+using ProfileCount = Function::ProfileCount;
+
----------------
This seems to be unused?


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:68
+
+  MF = &mf;
+  LocationDiscriminatorBBMap LDBM;
----------------
Why not just use the MF reference throughout? I don't see any uses in this function where a pointer is necessary.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:90
+        continue;
+      Location L = std::make_pair(DIL->getFilename(), LineNo);
+      unsigned Discriminator = DIL->getDiscriminator();
----------------
nit: I think this can simply be written as 
Location L{DIL->getFilename(), LineNo};

same for LocationDiscriminator LD below.



================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:118
+      }
+      Changed = true;
+    }
----------------
I think this should not be set unless we actually set the DILocation for the instruction (i.e. only when the else part is executed). Perhaps just add a continue on L110 inside the if() so that you can remove the else indentation block making it a bit easier to read as well.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:124
+    Module *M = MF->getFunction().getParent();
+    const std::string FSDisriminatorVar = "__llvm_fs_discriminator__";
+    if (!M->getGlobalVariable(FSDisriminatorVar)) {
----------------
Just use const char * since this string is immutable?
Also typo in FSDisriminatorVar -> FSDiscriminatorVar


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:169
 
+// An option that disables inserting FS-AFDO discrmintators before emit.
+static cl::opt<bool>
----------------
Can you also augment the comment to indicate where this option might be useful? It wasn't clear to me why this is needed.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:579
 
-  if (Header->getParent()->isDebugInfoForProfiling())
+  if (Header->getParent()->isDebugInfoForProfiling() && !EnableFSDiscriminator)
     for (BasicBlock *BB : L->getBlocks())
----------------
Can you add a comment here and below why this loop is skipped when FSDiscriminators are requested?


================
Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:8
+; CHECK: .loc    1 23 9 is_stmt 0 discriminator 268435457 # unroll.c:23:9
+; Check that variable __llvm_fs_discriminator__ is generated.
+; CHECK: .type   __llvm_fs_discriminator__, at object # @__llvm_fs_discriminator__
----------------
nit: I think ;; is used for comments. Adding a new line before the comment wouldn't hurt either.


================
Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:23
+
+define dso_local void @foo() #0 !dbg !29 {
+entry:
----------------
Since the IR for this test is non-trivial, can you include the c source and the invocation used to generate this? 
Probably good to do for the previous test as well.


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

https://reviews.llvm.org/D102246