[NFC] Move DroppedVariableStats to its own file and redesign it to be extensible. #115563
+455
−299
Conversation
This file contains 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
rastogishubham
requested review from
jmorse,
SLTozer,
felipepiovezan and
OCHyams
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Shubham Sandeep Rastogi (rastogishubham) ChangesMove DroppedVariableStats code to its own file and change the class to have an extensible design so that we can use it to add dropped statistics to MIR passes and the instruction selector. Patch is 34.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115563.diff 7 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/DroppedVariableStats.h b/llvm/include/llvm/CodeGen/DroppedVariableStats.h
new file mode 100644
index 00000000000000..d90dbe8bc72446
--- /dev/null
+++ b/llvm/include/llvm/CodeGen/DroppedVariableStats.h
@@ -0,0 +1,165 @@
+///===- DroppedVariableStats.h - Opt Diagnostics -*- C++ -*----------------===//
+///
+/// Part of the LLVM Project, under the Apache License v2.0 with LLVM
+/// Exceptions. See https://llvm.org/LICENSE.txt for license information.
+/// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+///
+///===---------------------------------------------------------------------===//
+/// \file
+/// Dropped Variable Statistics for Debug Information. Reports any number
+/// of #dbg_value that get dropped due to an optimization pass.
+///
+///===---------------------------------------------------------------------===//
+
+#ifndef LLVM_CODEGEN_DROPPEDVARIABLESTATS_H
+#define LLVM_CODEGEN_DROPPEDVARIABLESTATS_H
+
+#include "llvm/CodeGen/MachinePassManager.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PassInstrumentation.h"
+
+namespace llvm {
+
+/// A unique key that represents a #dbg_value.
+using VarID =
+ std::tuple<const DIScope *, const DIScope *, const DILocalVariable *>;
+
+/// A base class to collect and print dropped debug information variable
+/// statistics.
+class DroppedVariableStats {
+public:
+ DroppedVariableStats(bool DroppedVarStatsEnabled)
+ : DroppedVariableStatsEnabled(DroppedVarStatsEnabled) {
+ if (DroppedVarStatsEnabled)
+ llvm::outs()
+ << "Pass Level, Pass Name, Num of Dropped Variables, Func or "
+ "Module Name\n";
+ };
+ // We intend this to be unique per-compilation, thus no copies.
+ DroppedVariableStats(const DroppedVariableStats &) = delete;
+ void operator=(const DroppedVariableStats &) = delete;
+
+ void setup() {
+ DebugVariablesStack.push_back(
+ {DenseMap<const Function *, DebugVariables>()});
+ InlinedAts.push_back(
+ {DenseMap<StringRef, DenseMap<VarID, DILocation *>>()});
+ return;
+ }
+
+ void cleanup() {
+ DebugVariablesStack.pop_back();
+ InlinedAts.pop_back();
+ return;
+ }
+
+ bool getPassDroppedVariables() { return PassDroppedVariables; }
+
+protected:
+ bool PassDroppedVariables = false;
+ bool DroppedVariableStatsEnabled = false;
+
+ struct DebugVariables {
+ /// DenseSet of VarIDs before an optimization pass has run.
+ DenseSet<VarID> DebugVariablesBefore;
+ /// DenseSet of VarIDs after an optimization pass has run.
+ DenseSet<VarID> DebugVariablesAfter;
+ };
+
+ /// A stack of a DenseMap, that maps DebugVariables for every pass to an
+ /// llvm::Function. A stack is used because an optimization pass can call
+ /// other passes.
+ SmallVector<DenseMap<const Function *, DebugVariables>> DebugVariablesStack;
+
+ /// A DenseSet tracking whether a scope was visited before.
+ DenseSet<const DIScope *> VisitedScope;
+ /// A stack of DenseMaps, which map the name of an llvm::Function to a
+ /// DenseMap of VarIDs and their inlinedAt locations before an optimization
+ /// pass has run.
+ SmallVector<DenseMap<StringRef, DenseMap<VarID, DILocation *>>> InlinedAts;
+ /// Remove a dropped #dbg_value VarID from all Sets in the
+ /// DroppedVariablesBefore stack.
+ void removeVarFromAllSets(VarID Var, const Function *F) {
+ // Do not remove Var from the last element, it will be popped from the
+ // stack.
+ for (auto &DebugVariablesMap : llvm::drop_end(DebugVariablesStack))
+ DebugVariablesMap[F].DebugVariablesBefore.erase(Var);
+ }
+ /// Return true if \p Scope is the same as \p DbgValScope or a child scope of
+ /// \p DbgValScope, return false otherwise.
+ bool isScopeChildOfOrEqualTo(DIScope *Scope, const DIScope *DbgValScope);
+ /// Return true if \p InlinedAt is the same as \p DbgValInlinedAt or part of
+ /// the InlinedAt chain, return false otherwise.
+ bool isInlinedAtChildOfOrEqualTo(const DILocation *InlinedAt,
+ const DILocation *DbgValInlinedAt);
+};
+
+/// A class to collect and print dropped debug information due to LLVM IR
+/// optimization passes. After every LLVM IR pass is run, it will print how many
+/// #dbg_values were dropped due to that pass.
+class DroppedVariableStatsIR : public DroppedVariableStats {
+public:
+ DroppedVariableStatsIR(bool DroppedVarStatsEnabled)
+ : llvm::DroppedVariableStats(DroppedVarStatsEnabled) {}
+
+ void runBeforePass(Any IR) {
+ setup();
+ if (const auto *M = unwrapIR<Module>(IR))
+ return this->runOnModule(M, true);
+ if (const auto *F = unwrapIR<Function>(IR))
+ return this->runOnFunction(F, true);
+ }
+
+ void runAfterPass(StringRef P, Any IR) {
+ if (const auto *M = unwrapIR<Module>(IR))
+ runAfterPassModule(P, M);
+ else if (const auto *F = unwrapIR<Function>(IR))
+ runAfterPassFunction(P, F);
+ return cleanup();
+ }
+
+ void registerCallbacks(PassInstrumentationCallbacks &PIC);
+
+private:
+ void runAfterPassFunction(StringRef PassID, const Function *F) {
+ runOnFunction(F, false);
+ calculateDroppedVarStatsOnFunction(F, PassID, F->getName().str(),
+ "Function");
+ }
+
+ void runAfterPassModule(StringRef PassID, const Module *M) {
+ runOnModule(M, false);
+ calculateDroppedVarStatsOnModule(M, PassID, M->getName().str(), "Module");
+ }
+ /// Populate DebugVariablesBefore, DebugVariablesAfter, InlinedAts before or
+ /// after a pass has run to facilitate dropped variable calculation for an
+ /// llvm::Function.
+ void runOnFunction(const Function *F, bool Before);
+ /// Iterate over all Instructions in a Function and report any dropped debug
+ /// information.
+ void calculateDroppedVarStatsOnFunction(const Function *F, StringRef PassID,
+ std::string FuncOrModName,
+ std::string PassLevel);
+ /// Populate DebugVariablesBefore, DebugVariablesAfter, InlinedAts before or
+ /// after a pass has run to facilitate dropped variable calculation for an
+ /// llvm::Module. Calls runOnFunction on every Function in the Module.
+ void runOnModule(const Module *M, bool Before);
+ /// Iterate over all Functions in a Module and report any dropped debug
+ /// information. Will call calculateDroppedVarStatsOnFunction on every
+ /// Function.
+ void calculateDroppedVarStatsOnModule(const Module *M, StringRef PassID,
+ std::string FuncOrModName,
+ std::string PassLevel);
+
+ template <typename IRUnitT> static const IRUnitT *unwrapIR(Any IR) {
+ const IRUnitT **IRPtr = llvm::any_cast<const IRUnitT *>(&IR);
+ return IRPtr ? *IRPtr : nullptr;
+ }
+};
+
+} // namespace llvm
+
+#endif
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index 9301a12c740eec..12a34c099eaffe 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -19,6 +19,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
+#include "llvm/CodeGen/DroppedVariableStats.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/IR/BasicBlock.h"
#include "llvm/IR/DebugInfoMetadata.h"
@@ -579,83 +580,6 @@ class PrintCrashIRInstrumentation {
static void SignalHandler(void *);
};
-/// A class to collect and print dropped debug information variable statistics.
-/// After every LLVM IR pass is run, it will print how many #dbg_values were
-/// dropped due to that pass.
-class DroppedVariableStats {
-public:
- DroppedVariableStats(bool DroppedVarStatsEnabled) {
- if (DroppedVarStatsEnabled)
- llvm::outs()
- << "Pass Level, Pass Name, Num of Dropped Variables, Func or "
- "Module Name\n";
- };
- // We intend this to be unique per-compilation, thus no copies.
- DroppedVariableStats(const DroppedVariableStats &) = delete;
- void operator=(const DroppedVariableStats &) = delete;
-
- void registerCallbacks(PassInstrumentationCallbacks &PIC);
- void runBeforePass(StringRef PassID, Any IR);
- void runAfterPass(StringRef PassID, Any IR, const PreservedAnalyses &PA);
- void runAfterPassInvalidated(StringRef PassID, const PreservedAnalyses &PA);
- bool getPassDroppedVariables() { return PassDroppedVariables; }
-
-private:
- bool PassDroppedVariables = false;
- /// A unique key that represents a #dbg_value.
- using VarID =
- std::tuple<const DIScope *, const DIScope *, const DILocalVariable *>;
-
- struct DebugVariables {
- /// DenseSet of VarIDs before an optimization pass has run.
- DenseSet<VarID> DebugVariablesBefore;
- /// DenseSet of VarIDs after an optimization pass has run.
- DenseSet<VarID> DebugVariablesAfter;
- };
-
- /// A stack of a DenseMap, that maps DebugVariables for every pass to an
- /// llvm::Function. A stack is used because an optimization pass can call
- /// other passes.
- SmallVector<DenseMap<const Function *, DebugVariables>> DebugVariablesStack;
-
- /// A DenseSet tracking whether a scope was visited before.
- DenseSet<const DIScope *> VisitedScope;
- /// A stack of DenseMaps, which map the name of an llvm::Function to a
- /// DenseMap of VarIDs and their inlinedAt locations before an optimization
- /// pass has run.
- SmallVector<DenseMap<StringRef, DenseMap<VarID, DILocation *>>> InlinedAts;
-
- /// Iterate over all Functions in a Module and report any dropped debug
- /// information. Will call calculateDroppedVarStatsOnFunction on every
- /// Function.
- void calculateDroppedVarStatsOnModule(const Module *M, StringRef PassID,
- std::string FuncOrModName,
- std::string PassLevel);
- /// Iterate over all Instructions in a Function and report any dropped debug
- /// information.
- void calculateDroppedVarStatsOnFunction(const Function *F, StringRef PassID,
- std::string FuncOrModName,
- std::string PassLevel);
- /// Populate DebugVariablesBefore, DebugVariablesAfter, InlinedAts before or
- /// after a pass has run to facilitate dropped variable calculation for an
- /// llvm::Function.
- void runOnFunction(const Function *F, bool Before);
- /// Populate DebugVariablesBefore, DebugVariablesAfter, InlinedAts before or
- /// after a pass has run to facilitate dropped variable calculation for an
- /// llvm::Module. Calls runOnFunction on every Function in the Module.
- void runOnModule(const Module *M, bool Before);
- /// Remove a dropped #dbg_value VarID from all Sets in the
- /// DroppedVariablesBefore stack.
- void removeVarFromAllSets(VarID Var, const Function *F);
- /// Return true if \p Scope is the same as \p DbgValScope or a child scope of
- /// \p DbgValScope, return false otherwise.
- bool isScopeChildOfOrEqualTo(DIScope *Scope, const DIScope *DbgValScope);
- /// Return true if \p InlinedAt is the same as \p DbgValInlinedAt or part of
- /// the InlinedAt chain, return false otherwise.
- bool isInlinedAtChildOfOrEqualTo(const DILocation *InlinedAt,
- const DILocation *DbgValInlinedAt);
-};
-
/// This class provides an interface to register all the standard pass
/// instrumentations and manages their state (if any).
class StandardInstrumentations {
@@ -673,7 +597,7 @@ class StandardInstrumentations {
PrintCrashIRInstrumentation PrintCrashIR;
IRChangedTester ChangeTester;
VerifyInstrumentation Verify;
- DroppedVariableStats DroppedStats;
+ DroppedVariableStatsIR DroppedStatsIR;
bool VerifyEach;
diff --git a/llvm/lib/CodeGen/CMakeLists.txt b/llvm/lib/CodeGen/CMakeLists.txt
index 5a17944db0ae03..3eebeaaf04b3a2 100644
--- a/llvm/lib/CodeGen/CMakeLists.txt
+++ b/llvm/lib/CodeGen/CMakeLists.txt
@@ -50,6 +50,7 @@ add_llvm_component_library(LLVMCodeGen
DeadMachineInstructionElim.cpp
DetectDeadLanes.cpp
DFAPacketizer.cpp
+ DroppedVariableStats.cpp
DwarfEHPrepare.cpp
EarlyIfConversion.cpp
EdgeBundles.cpp
diff --git a/llvm/lib/CodeGen/DroppedVariableStats.cpp b/llvm/lib/CodeGen/DroppedVariableStats.cpp
new file mode 100644
index 00000000000000..0cdd679102a200
--- /dev/null
+++ b/llvm/lib/CodeGen/DroppedVariableStats.cpp
@@ -0,0 +1,150 @@
+///===- DroppedVariableStats.cpp - Opt Diagnostic -*- C++ -*---------------===//
+///
+/// Part of the LLVM Project, under the Apache License v2.0 with LLVM
+/// Exceptions. See https://llvm.org/LICENSE.txt for license information.
+/// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+///
+///===---------------------------------------------------------------------===//
+/// \file
+/// Dropped Variable Statistics for Debug Information. Reports any number
+/// of #dbg_value that get dropped due to an optimization pass.
+///
+///===---------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/DroppedVariableStats.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/Module.h"
+
+using namespace llvm;
+
+bool DroppedVariableStats::isScopeChildOfOrEqualTo(DIScope *Scope,
+ const DIScope *DbgValScope) {
+ while (Scope != nullptr) {
+ if (VisitedScope.find(Scope) == VisitedScope.end()) {
+ VisitedScope.insert(Scope);
+ if (Scope == DbgValScope) {
+ VisitedScope.clear();
+ return true;
+ }
+ Scope = Scope->getScope();
+ } else {
+ VisitedScope.clear();
+ return false;
+ }
+ }
+ return false;
+}
+
+bool DroppedVariableStats::isInlinedAtChildOfOrEqualTo(
+ const DILocation *InlinedAt, const DILocation *DbgValInlinedAt) {
+ if (DbgValInlinedAt == InlinedAt)
+ return true;
+ if (!DbgValInlinedAt)
+ return false;
+ if (!InlinedAt)
+ return false;
+ auto *IA = InlinedAt;
+ while (IA) {
+ if (IA == DbgValInlinedAt)
+ return true;
+ IA = IA->getInlinedAt();
+ }
+ return false;
+}
+
+void DroppedVariableStatsIR::runOnFunction(const Function *F, bool Before) {
+ auto &DebugVariables = DebugVariablesStack.back()[F];
+ auto &VarIDSet = (Before ? DebugVariables.DebugVariablesBefore
+ : DebugVariables.DebugVariablesAfter);
+ auto &InlinedAtsMap = InlinedAts.back();
+ auto FuncName = F->getName();
+ if (Before)
+ InlinedAtsMap.try_emplace(FuncName, DenseMap<VarID, DILocation *>());
+ VarIDSet = DenseSet<VarID>();
+ for (const auto &I : instructions(F)) {
+ for (DbgRecord &DR : I.getDbgRecordRange()) {
+ if (auto *Dbg = dyn_cast<DbgVariableRecord>(&DR)) {
+ auto *DbgVar = Dbg->getVariable();
+ auto DbgLoc = DR.getDebugLoc();
+ VarID Key{DbgVar->getScope(), DbgLoc->getInlinedAtScope(), DbgVar};
+ VarIDSet.insert(Key);
+ if (Before)
+ InlinedAtsMap[FuncName].try_emplace(Key, DbgLoc.getInlinedAt());
+ }
+ }
+ }
+}
+
+void DroppedVariableStatsIR::calculateDroppedVarStatsOnFunction(
+ const Function *F, StringRef PassID, std::string FuncOrModName,
+ std::string PassLevel) {
+ unsigned DroppedCount = 0;
+ StringRef FuncName = F->getName();
+ DebugVariables &DbgVariables = DebugVariablesStack.back()[F];
+ DenseSet<VarID> &DebugVariablesBeforeSet = DbgVariables.DebugVariablesBefore;
+ DenseSet<VarID> &DebugVariablesAfterSet = DbgVariables.DebugVariablesAfter;
+ DenseMap<VarID, DILocation *> &InlinedAtsMap = InlinedAts.back()[FuncName];
+ // Find an Instruction that shares the same scope as the dropped #dbg_value or
+ // has a scope that is the child of the scope of the #dbg_value, and has an
+ // inlinedAt equal to the inlinedAt of the #dbg_value or it's inlinedAt chain
+ // contains the inlinedAt of the #dbg_value, if such an Instruction is found,
+ // debug information is dropped.
+ for (VarID Var : DebugVariablesBeforeSet) {
+ if (DebugVariablesAfterSet.contains(Var))
+ continue;
+ const DIScope *DbgValScope = std::get<0>(Var);
+ for (const auto &I : instructions(F)) {
+ auto *DbgLoc = I.getDebugLoc().get();
+ if (!DbgLoc)
+ continue;
+
+ auto *Scope = DbgLoc->getScope();
+ if (isScopeChildOfOrEqualTo(Scope, DbgValScope)) {
+ if (isInlinedAtChildOfOrEqualTo(DbgLoc->getInlinedAt(),
+ InlinedAtsMap[Var])) {
+ // Found another instruction in the variable's scope, so there exists
+ // a break point at which the variable could be observed. Count it as
+ // dropped.
+ DroppedCount++;
+ break;
+ }
+ }
+ }
+ removeVarFromAllSets(Var, F);
+ }
+ if (DroppedCount > 0) {
+ llvm::outs() << PassLevel << ", " << PassID << ", " << DroppedCount << ", "
+ << FuncOrModName << "\n";
+ PassDroppedVariables = true;
+ } else
+ PassDroppedVariables = false;
+}
+
+void DroppedVariableStatsIR::runOnModule(const Module *M, bool Before) {
+ for (auto &F : *M)
+ runOnFunction(&F, Before);
+}
+
+void DroppedVariableStatsIR::calculateDroppedVarStatsOnModule(
+ const Module *M, StringRef PassID, std::string FuncOrModName,
+ std::string PassLevel) {
+ for (auto &F : *M) {
+ calculateDroppedVarStatsOnFunction(&F, PassID, FuncOrModName, PassLevel);
+ }
+}
+
+void DroppedVariableStatsIR::registerCallbacks(
+ PassInstrumentationCallbacks &PIC) {
+ if (!DroppedVariableStatsEnabled)
+ return;
+
+ PIC.registerBeforeNonSkippedPassCallback(
+ [this](StringRef P, Any IR) { return runBeforePass(IR); });
+ PIC.registerAfterPassCallback(
+ [this](StringRef P, Any IR, const PreservedAnalyses &PA) {
+ return runAfterPass(P, IR);
+ });
+ PIC.registerAfterPassInvalidatedCallback(
+ [this](StringRef P, const PreservedAnalyses &PA) { return cleanup(); });
+}
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index d4866a025c1b48..c79774ec03aef1 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -2460,7 +2460,7 @@ StandardInstrumentations::StandardInstrumentations(
PrintChanged == ChangePrinter::ColourDiffVerbose ||
PrintChanged == ChangePrinter::ColourDiffQuiet),
WebsiteChangeReporter(PrintChanged == ChangePrinter::DotCfgVerbose),
- Verify(DebugLogging), DroppedStats(DroppedVarStats),
+ Verify(DebugLogging), DroppedStatsIR(DroppedVarStats),
VerifyEach(VerifyEach) {}
PrintCrashIRInstrumentation *PrintCrashIRInstrumentation::CrashReporter =
@@ -2521,182 +2521,6 @@ void PrintCrashIRInstrumentation::registerCallbacks(
});
}
-void DroppedVariableStats::registerCallbacks(
- PassInstrumentationCallbacks &PIC) {
- if (!DroppedVarStats)
- return;
-
- PIC.registerBeforeNonSkippedPassCallback(
- [this](StringRef P, Any IR) { return this->runBeforePass(P, IR); });
- PIC.registerAfterPassCallback(
- [this](StringRef P, Any IR, const PreservedAnalyses &PA) {
- return this->runAfterPass(P, IR, PA);
- });
- PIC.registerAfterPassInvalidatedCallback(
- [this](StringRef P, const PreservedAnalyses &PA) {
- return this->runAfterPassInvalidated(P, PA);
- });
-}
-
-void DroppedVariableStats::runBeforePass(StringRef PassID, Any IR) {
- DebugVariablesStack.push_back({DenseMap<const Function *, DebugVariables>()});
- InlinedAts.push_back({DenseMap<StringRef, DenseMap<VarID, DILocation *>>()});
- if (auto *M = unwrapIR<Module>(IR))
- return this->runOnModule(M, true);
- if (auto *F = unwrapIR<Function>(IR))
- return this->runOnFunction(F, true);
- return;
-}
-
-void DroppedVariableStats::runOnFunction(const Function *F, bool Before) {
- auto &DebugVariables = DebugVariablesStack.back()[F];
- auto &VarIDSet = (Before ? DebugVariables.DebugVariablesBefore
- : DebugVariables.DebugVariablesAfter);
- auto &InlinedAtsMap = InlinedAts.back();
- auto FuncName = F->getName();
- if (Before)
- InlinedAtsMap.try_emplace(FuncName, DenseMap<VarID, DILocation *>());
- VarIDS...
[truncated]
|
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
3b908d0
to
3ef4dca
Compare
|
Hi @rastogishubham :)
Sorry to be a pain, would it be possible to split that change into either two commits (in this PR) or two PRs? That'd make it easier to review the changes to the class. |
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
3ef4dca
to
742e3df
Compare
|
@OCHyams Done! |
rastogishubham
changed the title
OCHyams
approved these changes
Nov 12, 2024
There was a problem hiding this comment.
Thanks, LGTM
(edit: though it possibly depends on this comment)
adrian-prantl
approved these changes
Nov 12, 2024
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
742e3df
to
acd9256
Compare
|
While I didn't use any templates and I didn't think any lambdas were necessary either, I did make the code more reusable by factoring out the common code into the base class, does this work @OCHyams @adrian-prantl |
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
acd9256
to
0e57047
Compare
OCHyams
reviewed
Nov 15, 2024
There was a problem hiding this comment.
There might be room for further code deduplication but the cost of the boilerplate might not be worth it, so I think this broadly LGTM, if @adrian-prantl is happy with this approach too. I've got some inline nits, mostly to do with comments and naming.
In future, please can you avoid squashing your changes when addressing reviewer feedback? It makes it more difficult to understand what has changed from the original patch (which means I find myself re-reviewing the whole thing rather than the changes). Especially with GitHub's tendency to swallow inline review comments. (It's fine to squish all your local commits between rounds of review ofc, and it's not a hard rule. There are times where it makes sense).
Comment on lines
108
to
112
| /// Check if a \p Var has been dropped or is a false positive. | ||
| bool wasDropped(DILocation *DbgLoc, const DIScope *Scope, | ||
| const DIScope *DbgValScope, | ||
| DenseMap<VarID, DILocation *> &InlinedAtsMap, VarID Var, | ||
| unsigned &DroppedCount); |
There was a problem hiding this comment.
Important to note in the comment that this also updates the "dropped" count IMO. Not sure how to reflect that in the function name too, but if it could be that would be nice.
There was a problem hiding this comment.
checkIfVarDroppedAndUpdateCount?
There was a problem hiding this comment.
How about updateDroppedCount (or ...Counter)? I feel like the bool return value + comment makes the rest of the functionality clear? ymmv.
| bool getPassDroppedVariables() { return PassDroppedVariables; } | ||
|
|
||
| protected: | ||
| void setup() { |
There was a problem hiding this comment.
Should we assert the containers are empty?
There was a problem hiding this comment.
No, because we are building up a stack of DenseMaps here, passes can call other passes, so we cannot assume that the stack will be empty for a pass that is called by another pass.
|
|
||
| void cleanup() { | ||
| DebugVariablesStack.pop_back(); | ||
| InlinedAts.pop_back(); |
There was a problem hiding this comment.
Same question about asserting assumptions here.
There was a problem hiding this comment.
Here however, we can assert to make sure the stacks are non-empty
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
0e57047
to
7fc75f2
Compare
OCHyams
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
LGTM - before committing can you please see if any of the fields in the base class can be made private instead of protected (I forgot to look at those while looking at the access specifiers for the member functions).
I'd also like to gently point out that you squashed your changes again (see my previous comment - this makes it more time consuming and difficult to review your updates).
|
@OCHyams sorry about the squashing, I missed that in your previous comment. I will make sure I keep that in mind for the future |
Change DroppedVariableStats to have an extensible design so that we can use it to add dropped statistics to MIR passes and the instruction selector.
rastogishubham
force-pushed
the
DroppedRefactor
branch
from
226edbc
to
b847006
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/8595 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/8597 Here is the relevant piece of the build log for the reference |
rastogishubham
added a commit
that referenced
this pull request
Nov 19, 2024
…it to be extensible. (#115563)" This reverts commit 2de7881. Reverted due to buildbot failure: unittests/IR/CMakeFiles/IRTests.dir/DroppedVariableStatsIRTest.cpp.o:DroppedVariableStatsIRTest.cpp:function llvm::DroppedVariableStatsIR::runAfterPass(llvm::StringRef, llvm::Any): error: undefined reference to 'llvm::DroppedVariableStatsIR::runOnModule(llvm::Module const*, bool)'
|
Had to revert with e914d97 because of buildbot failures |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Move DroppedVariableStats code to its own file and change the class to have an extensible design so that we can use it to add dropped statistics to MIR passes and the instruction selector.