-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[MLIR] Adopt LDBG() in EliminateBarriers.cpp (NFC) #155092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-mlir-gpu @llvm/pr-subscribers-llvm-support Author: Mehdi Amini (joker-eph) ChangesAlso add an extra optional TYPE argument to the LDBG() macro to make it easier to punctually overide DEBUG_TYPE. Full diff: https://github.com/llvm/llvm-project/pull/155092.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Support/DebugLog.h b/llvm/include/llvm/Support/DebugLog.h
index ead5dd2a4e8bd..c338ef5ec471c 100644
--- a/llvm/include/llvm/Support/DebugLog.h
+++ b/llvm/include/llvm/Support/DebugLog.h
@@ -39,15 +39,19 @@ namespace llvm {
// The `level` argument can be a literal integer, or a macro that evaluates to
// an integer.
//
+// An optional `type` argument can be provided to control the debug type. The
+// default type is DEBUG_TYPE. The `type` argument can be a literal string, or a
+// macro that evaluates to a string.
#define LDBG(...) _GET_LDBG_MACRO(__VA_ARGS__)(__VA_ARGS__)
// Helper macros to choose the correct macro based on the number of arguments.
-#define LDBG_FUNC_CHOOSER(_f1, _f2, ...) _f2
+#define LDBG_FUNC_CHOOSER(_f1, _f2, _f3, ...) _f3
#define LDBG_FUNC_RECOMPOSER(argsWithParentheses) \
LDBG_FUNC_CHOOSER argsWithParentheses
#define LDBG_CHOOSE_FROM_ARG_COUNT(...) \
- LDBG_FUNC_RECOMPOSER((__VA_ARGS__, LDBG_LOG_LEVEL, ))
-#define LDBG_NO_ARG_EXPANDER() , LDBG_LOG_LEVEL_1
+ LDBG_FUNC_RECOMPOSER( \
+ (__VA_ARGS__, LDBG_LOG_LEVEL_WITH_TYPE, LDBG_LOG_LEVEL, ))
+#define LDBG_NO_ARG_EXPANDER() , , LDBG_LOG_LEVEL_1
#define _GET_LDBG_MACRO(...) \
LDBG_CHOOSE_FROM_ARG_COUNT(LDBG_NO_ARG_EXPANDER __VA_ARGS__())
@@ -55,6 +59,8 @@ namespace llvm {
#define LDBG_LOG_LEVEL(LEVEL) \
DEBUGLOG_WITH_STREAM_AND_TYPE(llvm::dbgs(), LEVEL, DEBUG_TYPE)
#define LDBG_LOG_LEVEL_1() LDBG_LOG_LEVEL(1)
+#define LDBG_LOG_LEVEL_WITH_TYPE(LEVEL, TYPE) \
+ DEBUGLOG_WITH_STREAM_AND_TYPE(llvm::dbgs(), (LEVEL), (TYPE))
// We want the filename without the full path. We are using the __FILE__ macro
// and a constexpr function to strip the path prefix. We can avoid the frontend
diff --git a/mlir/lib/Dialect/GPU/Transforms/EliminateBarriers.cpp b/mlir/lib/Dialect/GPU/Transforms/EliminateBarriers.cpp
index 9bf11c7905aa1..d602bab9516c0 100644
--- a/mlir/lib/Dialect/GPU/Transforms/EliminateBarriers.cpp
+++ b/mlir/lib/Dialect/GPU/Transforms/EliminateBarriers.cpp
@@ -25,6 +25,7 @@
#include "mlir/Transforms/GreedyPatternRewriteDriver.h"
#include "llvm/ADT/TypeSwitch.h"
#include "llvm/Support/Debug.h"
+#include "llvm/Support/DebugLog.h"
namespace mlir {
#define GEN_PASS_DEF_GPUELIMINATEBARRIERS
@@ -37,9 +38,6 @@ using namespace mlir::gpu;
#define DEBUG_TYPE "gpu-erase-barriers"
#define DEBUG_TYPE_ALIAS "gpu-erase-barries-alias"
-#define DBGS() (llvm::dbgs() << '[' << DEBUG_TYPE << "] ")
-#define DBGS_ALIAS() (llvm::dbgs() << '[' << DEBUG_TYPE_ALIAS << "] ")
-
// The functions below provide interface-like verification, but are too specific
// to barrier elimination to become interfaces.
@@ -424,27 +422,18 @@ static bool maybeCaptured(Value v) {
/// everything. This seems sufficient to achieve barrier removal in structured
/// control flow, more complex cases would require a proper dataflow analysis.
static bool mayAlias(Value first, Value second) {
- DEBUG_WITH_TYPE(DEBUG_TYPE_ALIAS, {
- DBGS_ALIAS() << "checking aliasing between ";
- DBGS_ALIAS() << first << "\n";
- DBGS_ALIAS() << " and ";
- DBGS_ALIAS() << second << "\n";
- });
+ LDBG(1, DEBUG_TYPE_ALIAS)
+ << "checking aliasing between " << first << " and " << second;
first = getBase(first);
second = getBase(second);
- DEBUG_WITH_TYPE(DEBUG_TYPE_ALIAS, {
- DBGS_ALIAS() << "base ";
- DBGS_ALIAS() << first << "\n";
- DBGS_ALIAS() << " and ";
- DBGS_ALIAS() << second << "\n";
- });
+ LDBG(1, DEBUG_TYPE_ALIAS) << "base " << first << " and " << second;
// Values derived from the same base memref do alias (unless we do a more
// advanced analysis to prove non-overlapping accesses).
if (first == second) {
- DEBUG_WITH_TYPE(DEBUG_TYPE_ALIAS, DBGS_ALIAS() << "-> do alias!\n");
+ LDBG(1, DEBUG_TYPE_ALIAS) << "-> do alias!";
return true;
}
@@ -493,7 +482,7 @@ static bool mayAlias(Value first, Value second) {
return false;
// Otherwise, conservatively assume aliasing.
- DEBUG_WITH_TYPE(DEBUG_TYPE_ALIAS, DBGS_ALIAS() << "-> may alias!\n");
+ LDBG(1, DEBUG_TYPE_ALIAS) << "-> may alias!";
return true;
}
@@ -567,20 +556,16 @@ haveConflictingEffects(ArrayRef<MemoryEffects::EffectInstance> beforeEffects,
continue;
// Other kinds of effects create a conflict, e.g. read-after-write.
- LLVM_DEBUG(
- DBGS() << "found a conflict between (before): " << before.getValue()
- << " read:" << isa<MemoryEffects::Read>(before.getEffect())
- << " write:" << isa<MemoryEffects::Write>(before.getEffect())
- << " alloc:"
- << isa<MemoryEffects::Allocate>(before.getEffect()) << " free:"
- << isa<MemoryEffects::Free>(before.getEffect()) << "\n");
- LLVM_DEBUG(
- DBGS() << "and (after): " << after.getValue()
- << " read:" << isa<MemoryEffects::Read>(after.getEffect())
- << " write:" << isa<MemoryEffects::Write>(after.getEffect())
- << " alloc:" << isa<MemoryEffects::Allocate>(after.getEffect())
- << " free:" << isa<MemoryEffects::Free>(after.getEffect())
- << "\n");
+ LDBG() << "found a conflict between (before): " << before.getValue()
+ << " read:" << isa<MemoryEffects::Read>(before.getEffect())
+ << " write:" << isa<MemoryEffects::Write>(before.getEffect())
+ << " alloc:" << isa<MemoryEffects::Allocate>(before.getEffect())
+ << " free:" << isa<MemoryEffects::Free>(before.getEffect());
+ LDBG() << "and (after): " << after.getValue()
+ << " read:" << isa<MemoryEffects::Read>(after.getEffect())
+ << " write:" << isa<MemoryEffects::Write>(after.getEffect())
+ << " alloc:" << isa<MemoryEffects::Allocate>(after.getEffect())
+ << " free:" << isa<MemoryEffects::Free>(after.getEffect());
return true;
}
}
@@ -595,8 +580,8 @@ class BarrierElimination final : public OpRewritePattern<BarrierOp> {
LogicalResult matchAndRewrite(BarrierOp barrier,
PatternRewriter &rewriter) const override {
- LLVM_DEBUG(DBGS() << "checking the necessity of: " << barrier << " "
- << barrier.getLoc() << "\n");
+ LDBG() << "checking the necessity of: " << barrier << " "
+ << barrier.getLoc();
SmallVector<MemoryEffects::EffectInstance> beforeEffects;
getEffectsBefore(barrier, beforeEffects, /*stopAtBarrier=*/true);
@@ -605,14 +590,12 @@ class BarrierElimination final : public OpRewritePattern<BarrierOp> {
getEffectsAfter(barrier, afterEffects, /*stopAtBarrier=*/true);
if (!haveConflictingEffects(beforeEffects, afterEffects)) {
- LLVM_DEBUG(DBGS() << "the surrounding barriers are sufficient, removing "
- << barrier << "\n");
+ LDBG() << "the surrounding barriers are sufficient, removing " << barrier;
rewriter.eraseOp(barrier);
return success();
}
- LLVM_DEBUG(DBGS() << "barrier is necessary: " << barrier << " "
- << barrier.getLoc() << "\n");
+ LDBG() << "barrier is necessary: " << barrier << " " << barrier.getLoc();
return failure();
}
};
|
DBGS_ALIAS() << " and "; | ||
DBGS_ALIAS() << second << "\n"; | ||
}); | ||
LDBG(1, DEBUG_TYPE_ALIAS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is following default arguments are last kind of C++ style, but I think the opposite reads nicer here:
LDBG(DEBUG_TYPE, 1)
As the former is the higher-level filter and the latter the level within that. Thinking out loud, I wonder: could we even overload on type here? (so LDBG(DEBUG_TYPE)
could work too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me look at the overload idea, I like it :)
I wanted to preserve the more common case where you have a single DEBUG_TYPE in a file and you just want to adjust for the log level LDBG(2)
instead of having to be explicit about the DEBUG_TYPE just to change the log level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK it's done, PTAL :)
Also add an extra optional TYPE argument to the LDBG() macro to make it easier to punctually overide DEBUG_TYPE.
130822e
to
9e859fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks
Also add an extra optional TYPE argument to the LDBG() macro to make it easier to punctually overide DEBUG_TYPE.