Add support for collection, recording and analysis of SIL optimizer counters #11813
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swiftix
force-pushed
the
optimizer-counters
branch
from
2fa0f38 to
cff4b05
Compare
gottesmm
reviewed
Sep 8, 2017
Contributor
gottesmm
left a comment
gottesmm
left a comment
There was a problem hiding this comment.
My comments I forgot to send yesterday. I am going back through it again though.
Contributor
There was a problem hiding this comment.
Does this need to be a std::string? Why not return a StringRef?
Contributor
There was a problem hiding this comment.
Can you make this conditional on running with the given option enabled?
Contributor
Author
There was a problem hiding this comment.
Are you afraid of std::chrono::system_clock::now() being an expensive call?
BTW, the matching start time computation, which is a couple of lines above, is always performed unconditionally:
llvm::sys::TimePoint<> StartTime = std::chrono::system_clock::now();
gottesmm
reviewed
Sep 8, 2017
Contributor
gottesmm
left a comment
gottesmm
left a comment
There was a problem hiding this comment.
A bunch of small comments. A bigger ask though:
Can you add a lit *.sil that shows all of this working. This will ensure that this doesn't break. I mean something that shows the optimizer producing the appropriate counters.
docs/OptimizerCountersAnalysis.md
Outdated
docs/OptimizerCountersAnalysis.md
Outdated
Contributor
There was a problem hiding this comment.
This could be made clearer. It almost sounds like the counters are always produced by the compiler. Also, you sort of just go straight into talking about optimizer counters, without saying "what an optimizer counter is".
docs/OptimizerCountersAnalysis.md
Outdated
Contributor
There was a problem hiding this comment.
I think you messed up the formatting here.
Contributor
There was a problem hiding this comment.
Can you use a doxygen comment here: I.e.:
/// Get the name of the current optimization stage.
///
/// This is useful for debugging. Please keep this out of line so that lldb is able to always find it.
Contributor
There was a problem hiding this comment.
Also, isn't StatsOnlyFunctionNamesPattern a String as well?
Contributor
Author
There was a problem hiding this comment.
It is inside an anonymous namespace anyways. So, it cannot be referenced from outside.
Contributor
There was a problem hiding this comment.
llvm::SmallString<64>? (I forgot if 32 or 64 is the limit), I doubt any name of an instruction is more than 20 characters).
Contributor
There was a problem hiding this comment.
Why not duplicate this into the if statement and return early. The else statement is pretty long here and you could then get rid of the indentation. I don't have a strong opinion on this though.
Contributor
|
The mail comment is missing 1 change that is on the website, the #include of <iostream>. See the following comment:
#11813 (comment)
… On Sep 8, 2017, at 2:41 PM, Michael Gottesman ***@***.***> wrote:
@gottesmm commented on this pull request.
A bunch of small comments. A bigger ask though:
Can you add a lit *.sil that shows all of this working. This will ensure that this doesn't break. I mean something that shows the optimizer producing the appropriate counters.
In docs/OptimizerCountersAnalysis.md <#11813 (comment)>:
> @@ -0,0 +1,258 @@
+## Collecting and analyzing optimizer counters
Can you add a ToC?
In docs/OptimizerCountersAnalysis.md <#11813 (comment)>:
> @@ -0,0 +1,258 @@
+## Collecting and analyzing optimizer counters
+
+This document describes how you collect and analyze the counters produced by
This could be made clearer. It almost sounds like the counters are always produced by the compiler. Also, you sort of just go straight into talking about optimizer counters, without saying "what an optimizer counter is".
In docs/OptimizerCountersAnalysis.md <#11813 (comment)>:
> +
+### The format of the recorded optimizer counters
+
+The counters are recorded using a simple CSV (comma separated value) format.
+Each line represents a single counter value or a counter value change.
+
+For counter value updates, the CSV line looks like this:
+ * `Kind, CounterName, StageName, TransformName, TransformPassNumber,
+ DeltaValue, OldCounterValue, NewCounterValue, Duration, Symbol`
+
+And for counter updates it looks like this:
+ * `Kind, CounterName, StageName, TransformName, TransformPassNumber,
+ CounterValue, Duration, Symbol`
+
+ where:
+ * `Kind` is one of `function`, `module`, `function_history`.
I think you messed up the formatting here.
In include/swift/SILOptimizer/PassManager/PassManager.h <#11813 (comment)>:
> @@ -214,6 +214,9 @@ class SILPassManager {
// Sets the name of the current optimization stage used for debugging.
void setStageName(llvm::StringRef NextStage = "");
+ // Get the name of the current optimization stage used for debugging.
Can you use a doxygen comment here: I.e.:
/// Get the name of the current optimization stage.
///
/// This is useful for debugging. Please keep this out of line so that lldb is able to always find it.
In lib/SILOptimizer/PassManager/PassManager.cpp <#11813 (comment)>:
> @@ -338,8 +341,8 @@ void SILPassManager::runPassOnFunction(SILFunctionTransform *SFT,
assert(analysesUnlocked() && "Expected all analyses to be unlocked!");
Mod->removeDeleteNotificationHandler(SFT);
+ auto Delta = (std::chrono::system_clock::now() - StartTime).count();
Ok.
In utils/optimizer_counters_to_sql.py <#11813 (comment)>:
> +
+
+# A simple class to connect to a DB and store the statistics from a CSV
+# (comma separated values) file with optimizer counters there.
+#
+# See OptimizerCountersAnalysis.md for more details about working with
+# produced SQLite database and analyzing the collected optimizer counters.
+class OptStatsDB(object):
+ def __init__(self, dbname):
+ try:
+ # Establish a connection to a DB.
+ con = lite.connect(dbname)
+ self.con = con
+ cur = con.cursor()
+ self.cur = cur
+ # FIXME:
Can you fix the formatting here.
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> @@ -0,0 +1,970 @@
+//===-- OptimizerStatsUtils.cpp - Utils for collecting stats -*- C++ ---*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
This doxygen comment is beautiful!
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +#include <iostream>
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Process.h"
+#include "swift/SIL/SILValue.h"
+#include "swift/SIL/SILVisitor.h"
+#include "swift/SILOptimizer/Analysis/Analysis.h"
+#include "swift/SILOptimizer/PassManager/PassManager.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/OptimizerStatsUtils.h"
+using namespace swift;
+
+namespace {
+
+/// The total number of different kinds of SILInstructions.
+constexpr int SILInstructionsNum = int(ValueKind::Last_SILInstruction) + 1;
unsigned?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +#include "llvm/Support/Process.h"
+#include "swift/SIL/SILValue.h"
+#include "swift/SIL/SILVisitor.h"
+#include "swift/SILOptimizer/Analysis/Analysis.h"
+#include "swift/SILOptimizer/PassManager/PassManager.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/OptimizerStatsUtils.h"
+using namespace swift;
+
+namespace {
+
+/// The total number of different kinds of SILInstructions.
+constexpr int SILInstructionsNum = int(ValueKind::Last_SILInstruction) + 1;
+
+/// A set of counters, one per SILInstruction kind.
+typedef SmallVector<int, SILInstructionsNum> InstructionCounts;
using?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +/// The total number of different kinds of SILInstructions.
+constexpr int SILInstructionsNum = int(ValueKind::Last_SILInstruction) + 1;
+
+/// A set of counters, one per SILInstruction kind.
+typedef SmallVector<int, SILInstructionsNum> InstructionCounts;
+
+/// Map a SILInstruction name to its SILInstructionKind.
+SILInstructionKind getSILInstructionKind(StringRef InstName) {
+#define INST(Id, Parent, TextualName, MemoryBehavior, ReleasingBehavior) \
+ if (InstName == #TextualName) \
+ return SILInstructionKind::Id;
+
+#include "swift/SIL/SILNodes.def"
+
+#ifdef NDEBUG
+ std::cerr
Please do not use std::cerr. Please use:
llvm::errs()
Also, there is a typo:
insruction => instruction
Finally, why not just create an unreachable that always asserts irregardless of assertions. I have wanted that before.
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + if (InstName == #TextualName) \
+ return SILInstructionKind::Id;
+
+#include "swift/SIL/SILNodes.def"
+
+#ifdef NDEBUG
+ std::cerr
+ << "Unknown insruction name used by the -sil-stats-only-instructions\n";
+ abort();
+#endif
+ llvm_unreachable(
+ "Unknown insruction name used by the -sil-stats-only-instructions");
+}
+
+/// Map SILInstructionKind to a corresponding SILInstruction name.
+const char *getSILInstructionName(SILInstructionKind Kind) {
Don't we already have this?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +
+namespace {
+
+/// The total number of different kinds of SILInstructions.
+constexpr int SILInstructionsNum = int(ValueKind::Last_SILInstruction) + 1;
+
+/// A set of counters, one per SILInstruction kind.
+typedef SmallVector<int, SILInstructionsNum> InstructionCounts;
+
+/// Map a SILInstruction name to its SILInstructionKind.
+SILInstructionKind getSILInstructionKind(StringRef InstName) {
+#define INST(Id, Parent, TextualName, MemoryBehavior, ReleasingBehavior) \
+ if (InstName == #TextualName) \
+ return SILInstructionKind::Id;
+
+#include "swift/SIL/SILNodes.def"
I think we already have this, no?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +/// Kind, CounterName, StageName, TransformName, TransformPassNumber,
+/// DeltaValue, OldCounterValue, NewCounterValue, Duration, Symbol
+///
+/// For counter updates it looks like this:
+/// Kind, CounterName, StageName, TransformName, TransformPassNumber,
+/// CounterValue, Duration, Symbol
+///
+/// where Kind is one of "function", "module", "function_history",
+/// CounterName is one of "block", "inst", "function", "memory",
+/// Symbol is e.g. the name of a function.
+/// StageName and TransformName are the names of the current optimizer
+/// pipeline stage and current transform.
+/// Duration is the duration of the transformation.
+//===----------------------------------------------------------------------===//
+
+#include <iostream>
This violates LLVM coding style: http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden <http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden>
You should be using llvm::errs() below.
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +
+public:
+ StatsOnlyInstructionsOpt() : ComputeInstCounts(SILInstructionsNum) {}
+
+ void operator=(const std::string &Val) {
+ if (Val.empty())
+ return;
+ if (Val == "all") {
+ for (auto &Inst : ComputeInstCounts) {
+ Inst = 1;
+ }
+ InstCountsNum = ComputeInstCounts.size();
+ return;
+ }
+ SmallVector<StringRef, 8> statsInstNames;
+ StringRef(Val).split(statsInstNames, ',', -1, false);
Can't you use the incremental form of split:
http://llvm.org/doxygen/classllvm_1_1StringRef.html#a74783910a317456a47477f21dc8a73de <http://llvm.org/doxygen/classllvm_1_1StringRef.html#a74783910a317456a47477f21dc8a73de>
Then you don't need to use a small vector.
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + return BlockCount == rhs.BlockCount && InstCount == rhs.InstCount;
+ }
+
+ bool operator!=(const FunctionStat &rhs) const { return !(*this == rhs); }
+
+ void dump() { print(llvm::errs()); }
+};
+
+/// Stats a single SIL module.
+struct ModuleStat {
+ // Total number of SILFunctions in the current SILModule.
+ int FunctionCount = 0;
+ // Total number of SILBasicBlocks in the current SILModule.
+ int BlockCount = 0;
+ // Total number of SILInstructions in the current SILModule.
+ int InstCount = 0;
Doxygen comments all through this struct block.
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + // Total number of SILInstructions deleted since the beginning of the current
+ // compilation.
+ int DeletedInstCount = 0;
+ // Instruction counts per SILInstruction kind.
+ InstructionCounts InstCounts;
+
+ ModuleStat() : InstCounts(SILInstructionsNum) {}
+
+ // Add the stats for a given function to the total module stats.
+ void addFunctionStat(FunctionStat &Stat) {
+ BlockCount += Stat.BlockCount;
+ InstCount += Stat.InstCount;
+ ++FunctionCount;
+ if (!StatsOnlyInstructionsOptLoc.getInstCountsNum())
+ return;
+ for (int i = 0, e = InstCounts.size(); i < e; ++i) {
Have you ever used the indices helper? It makes stuff like this cleaner:
for (unsigned i : indices(InstCounts)) {
...
}
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + return PM.getStageName();
+ }
+
+ SILModule &getModule() {
+ return M;
+ }
+
+ SILPassManager &getPassManager() {
+ return PM;
+ }
+};
+
+/// Aggregated statistics for the whole SILModule and all SILFunctions belonging
+/// to it.
+class AccumulatedOptimizerStats {
+ typedef llvm::DenseMap<const SILFunction *, FunctionStat> FunctionStats;
using?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +
+/// The output stream to be used for writing the collected statistics.
+/// Use the unique_ptr to ensure that the file is properly closed upon
+/// exit.
+std::unique_ptr<llvm::raw_ostream, std::function<void(llvm::raw_ostream *)>>
+ stats_output_stream;
+
+/// Return the output streamm to be used for logging the collected statistics.
+llvm::raw_ostream &stats_os() {
+ // Initialize the stream if it is not initialized yet.
+ if (!stats_output_stream) {
+ // If there is user-defined output file name, use it.
+ if (!SILStatsOutputFile.empty()) {
+ // Try to open the file.
+ std::error_code EC;
+ llvm::raw_fd_ostream *fd_stream = new llvm::raw_fd_ostream(
std::unique_ptr + move?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> +
+ stats_os() << Symbol;
+ stats_os() << "\n";
+}
+
+/// Check if a function name matches the pattern provided by the user.
+bool isMatchingFunction(SILFunction *F, bool shouldHaveNamePattern = false) {
+ auto FuncName = F->getName();
+ // Is it an exact string match?
+ if (!StatsOnlyFunctionName.empty())
+ return F->getName() == StatsOnlyFunctionName;
+
+ // Does the name contain a user-defined substring?
+ if (!StatsOnlyFunctionsNamePattern.empty()) {
+ StringRef Pattern(StatsOnlyFunctionsNamePattern);
+ return FuncName.find(Pattern) != StringRef::npos;
StringRef::contains?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + // Is it an exact string match?
+ if (!StatsOnlyFunctionName.empty())
+ return F->getName() == StatsOnlyFunctionName;
+
+ // Does the name contain a user-defined substring?
+ if (!StatsOnlyFunctionsNamePattern.empty()) {
+ StringRef Pattern(StatsOnlyFunctionsNamePattern);
+ return FuncName.find(Pattern) != StringRef::npos;
+ }
+
+ return shouldHaveNamePattern ? true : false;
+}
+
+/// Compute the delta between the old and new values.
+/// Return it as a percent value.
+double computeDelta(int Old, int New) {
static?
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + OldStat.DeletedInstCount, NewStat.DeletedInstCount,
+ Ctx);
+ }
+
+ /// Dump collected instruction counts.
+ if (!StatsOnlyInstructionsOptLoc.getInstCountsNum())
+ return;
+
+ for (int i = 0, e = SILInstructionsNum; i < e; ++i) {
+ // Do not print anything, if there is no change.
+ if (OldStat.InstCounts[i] == NewStat.InstCounts[i])
+ continue;
+ SILInstructionKind Kind = SILInstructionKind(i);
+ if (!StatsOnlyInstructionsOptLoc.shouldComputeInstCount(Kind))
+ continue;
+ std::string CounterName = "inst_";
llvm::SmallString<64>? (I forgot if 32 or 64 is the limit), I doubt any name of an instruction is more than 20 characters).
In lib/SILOptimizer/Utils/OptimizerStatsUtils.cpp <#11813 (comment)>:
> + }
+
+ // Process added functions.
+ while (!AddedFuncs.empty()) {
+ auto *F = AddedFuncs.back();
+ AddedFuncs.pop_back();
+ auto &FuncStat = getFunctionStat(F);
+ FunctionStat OldFuncStat;
+ FunctionStat NewFuncStat(F);
+ processFuncStatsChanges(F, OldFuncStat, NewFuncStat, Ctx);
+ NewModStat.addFunctionStat(NewFuncStat);
+ FuncStat = NewFuncStat;
+ }
+ }
+
+ NewModStat.addMemoryStat();
Why not duplicate this into the if statement and return early. The else statement is pretty long here and you could then get rid of the indentation. I don't have a strong opinion on this though.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#11813 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAee32j_gLu5CJCB1Wuoc_WkkYH_yswkks5sgbR4gaJpZM4PQmhe>.
|
gottesmm
reviewed
Sep 8, 2017
docs/OptimizerCountersAnalysis.md
Outdated
Contributor
There was a problem hiding this comment.
Can you add indentation here to make this more readable?
Contributor
Author
|
@swift-ci please smoke test |
3 similar comments
Contributor
Author
|
@swift-ci please smoke test |
Contributor
Author
|
@swift-ci please smoke test |
Contributor
Author
|
@swift-ci please smoke test |
Contributor
Author
|
@swift-ci please test |
1 similar comment
Contributor
Author
|
@swift-ci please test |
Contributor
|
Build failed |
swiftix
force-pushed
the
optimizer-counters
branch
from
2f55945 to
c3717e2
Compare
Contributor
Author
|
@swift-ci please test |
Contributor
|
Build failed |
Contributor
Author
|
@swift-ci please test |
Contributor
|
Build failed |
Contributor
Author
|
@swift-ci please smoke test Linux |
1 similar comment
Contributor
Author
|
@swift-ci please smoke test Linux |
swiftix
force-pushed
the
optimizer-counters
branch
from
c3717e2 to
ae393ca
Compare
Contributor
Author
|
@swift-ci please smoke test |
1 similar comment
Contributor
Author
|
@swift-ci please smoke test |
Contributor
Author
|
Linux lib dispatch errors seem to be unrelated: Does anyone has an idea what it could be? Is it a known issue? |
Contributor
Author
|
@swift-ci please smoke test Linux |
This patch implements collection and dumping of statistics about SILModules, SILFunctions and memory consumption during the execution of SIL optimization pipelines. The following statistics can be collected: * For SILFunctions: the number of SIL basic blocks, the number of SIL instructions, the number of SIL instructions of a specific kind, duration of a pass * For SILModules: the number of SIL basic blocks, the number of SIL instructions, the number of SIL instructions of a specific kind, the number of SILFunctions, the amount of memory used by the compiler, duration of a pass By default, any collection of statistics is disabled to avoid affecting compile times. One can enable the collection of statistics and dumping of these statistics for the whole SILModule and/or for SILFunctions. To reduce the amount of produced data, one can set thresholds in such a way that changes in the statistics are only reported if the delta between the old and the new values are at least X%. The deltas are computed as using the following formula: Delta = (NewValue - OldValue) / OldValue Thresholds provide a simple way to perform a simple filtering of the collected statistics during the compilation. But if there is a need for a more complex analysis of collected data (e.g. aggregation by a pipeline stage or by the type of a transformation), it is often better to dump as much data as possible into a file using e.g. -sil-stats-dump-all -sil-stats-modules -sil-stats-functions and then e.g. use the helper scripts to store the collected data into a database and then perform complex queries on it. Many kinds of analysis can be then formulated pretty easily as SQL queries.
…SILValue/SILInstruction and its mnemonic name Use it for the stats collection, but also for SIL printing.
It described the overall idea behind the optimizer counters, how to collect and record them and how to analyze them
swiftix
force-pushed
the
optimizer-counters
branch
from
ae393ca to
68ba040
Compare
Contributor
Author
|
@swift-ci please smoke test Linux |
1 similar comment
Contributor
Author
|
@swift-ci please smoke test Linux |
Contributor
Author
|
@swift-ci please smoke test |
Contributor
|
Nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements collection and dumping of statistics about SILModules, SILFunctions and memory consumption during the execution of SIL optimization pipelines.
The following statistics can be collected:
By default, any collection of statistics is disabled to avoid affecting compile times.
One can enable the collection of statistics and dumping of these statistics for the whole SILModule and/or for SILFunctions.
To reduce the amount of produced data, one can set thresholds in such a way that changes in the statistics are only reported if the delta between the old and the new values are at least X%. The deltas are computed using the following formula:
Delta = 100 * (NewValue - OldValue) / OldValueThresholds provide a simple way to perform a simple filtering of the collected statistics during the compilation. But if there is a need for a more complex analysis of collected data (e.g. aggregation by a pipeline stage or by the type of a transformation), it is often better to dump as much data as possible into a file using e.g.
-sil-stats-dump-all -sil-stats-modules -sil-stats-functionsand then e.g. use the helper scripts to store the collected data into a database and then perform complex queries on it. Many kinds of analysis can be then formulated pretty easily as SQL queries.