[PATCH] D109934: [llvm-profgen] Add duplication factor for line-number based profile
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 24 12:20:49 PDT 2021
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
More information about the llvm-commits
mailing list