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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
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
----------------
wenlei wrote:
> 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. 
I see, After adding the bigger test case(quick sort program), I found that will naturally  generate a duplication factor case. So I just removed this test.


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


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:220
+    Frame.Callsite.Discriminator =
+        ProfileGenerator::getDiscriminator(Frame.Callsite.Discriminator);
+  }
----------------
wenlei wrote:
> 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). 
Here the code is guaranteed only used by line-number based because this is the function to generate the `StringBasedCtxKey`.

```
std::shared_ptr<StringBasedCtxKey> FrameStack::getContextKey() {
  std::shared_ptr<StringBasedCtxKey> KeyStr =
      std::make_shared<StringBasedCtxKey>();
  KeyStr->Context = Binary->getExpandedContext(Stack, KeyStr->WasLeafInlined);
  ... 
}
```


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:535-538
-    } else {
-      Discriminator = DILocation::getBaseDiscriminatorFromDiscriminator(
-          CallerFrame.Discriminator,
-          /* IsFSDiscriminator */ false);
----------------
wenlei wrote:
> 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.
I checked that our internal tools also used duplication factor this way, i. e decoding it when we need it in profile generation. 

Alternatively,  we can also have a hash map like {offset, duplication factor} offset2DuplicationFactorMap which can be built on-demand as well, which one do you think is better?





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109934