[PATCH] D109934: [llvm-profgen] Add duplication factor for line-number based profile

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/duplication-factor.test:6
+
+; Test when the duplication factor is 10
+
----------------
Can you comment on the profile line where the duplication factor is being changed to 10? and what's the count if there's no duplication factor?


================
Comment at: llvm/test/tools/llvm-profgen/duplication-factor.test:30
+; clang -O3 -g -fdebug-info-for-profiling test.c -emit-llvm -S -o test.ll
+; # Manually update the discriminator for a duplication-factor whose value is 10 in test.ll. For example: a discriminator value 2(0010) will be changed to 2562(101000000010)
+; clang test.ll
----------------
I think it's better to have a test that can naturally generate duplication factor. That way it's easier to update the test case in the future if needed. To do that we can grab one of the test cases from loop unrolling, and if we enable fdebug-info-for-profiling, i think we should see duplication factor. 


================
Comment at: llvm/test/tools/llvm-profgen/duplication-factor.test:53
+}
+~
----------------
is this intentional?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:503
       continue;
+    uint64_t DC = Count * getDuplicationFactor(LeafLoc->Callsite.Discriminator);
     FunctionProfile.addCalledTargetSamples(LeafLoc->Callsite.LineOffset,
----------------
nit: `Count *= getDuplicationFactor(LeafLoc->Callsite.Discriminator)`? Same for other places.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:220
+    Frame.Callsite.Discriminator =
+        ProfileGenerator::getDiscriminator(Frame.Callsite.Discriminator);
+  }
----------------
Current this function is only used for dwarf based profile, but is it guaranteed that this path will never be used by pseudo probe? we will need to handle discriminator differently for probe case (extractProbeIndex). 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:535-538
-    } else {
-      Discriminator = DILocation::getBaseDiscriminatorFromDiscriminator(
-          CallerFrame.Discriminator,
-          /* IsFSDiscriminator */ false);
----------------
By removing this part, we will have duplication factor as part of location in NameStrings and call stack, is that what we want? Logically, duplication factor does not belong to location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109934