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

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added a comment.

snehasish, thanks for the reviewing comments! I'll integrate your comments and update the patch.



================
Comment at: llvm/include/llvm/Support/FSAFDODiscriminator.h:41
+
+#ifndef NDEBUG
+// Given a discriminator value DiscriminatorVal, return the number of buckets
----------------
snehasish wrote:
> 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?
Yes. That are for the FS profile loader. I'll remove them and add later when they are used.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:24
+using namespace llvm;
+using ProfileCount = Function::ProfileCount;
+
----------------
snehasish wrote:
> This seems to be unused?
This is left over from refactor. I'll remove this.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:68
+
+  MF = &mf;
+  LocationDiscriminatorBBMap LDBM;
----------------
snehasish wrote:
> Why not just use the MF reference throughout? I don't see any uses in this function where a pointer is necessary.
This code has been changed quite a bit over time. But you are right that now we can use reference directly. I will make the change.


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


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:118
+      }
+      Changed = true;
+    }
----------------
snehasish wrote:
> 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.
It's rarely that cloneWithDiscriminator return empty DIL. But you are right here -- I should only set in the else clause.
I will fix this.


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


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:169
 
+// An option that disables inserting FS-AFDO discrmintators before emit.
+static cl::opt<bool>
----------------
snehasish wrote:
> Can you also augment the comment to indicate where this option might be useful? It wasn't clear to me why this is needed.
This is for debuging and tuning purpose. I don't think this gonna hurt anything. If later we found this rarely used, we can remove by then.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:579
 
-  if (Header->getParent()->isDebugInfoForProfiling())
+  if (Header->getParent()->isDebugInfoForProfiling() && !EnableFSDiscriminator)
     for (BasicBlock *BB : L->getBlocks())
----------------
snehasish wrote:
> Can you add a comment here and below why this loop is skipped when FSDiscriminators are requested?
This is just to disable the existing multiply factor in discriminators.
I will add a comment here (and a few other places like this).


================
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__
----------------
snehasish wrote:
> nit: I think ;; is used for comments. Adding a new line before the comment wouldn't hurt either.
will fix.


================
Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:23
+
+define dso_local void @foo() #0 !dbg !29 {
+entry:
----------------
snehasish wrote:
> 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.
Yes. This is a good idea.


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

https://reviews.llvm.org/D102246