Skip to content

Conversation

joker-eph
Copy link
Collaborator

Also add an extra optional TYPE argument to the LDBG() macro to make it easier to punctually overide DEBUG_TYPE.

@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2025

@llvm/pr-subscribers-mlir-gpu
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-llvm-support

Author: Mehdi Amini (joker-eph)

Changes

Also 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:

  • (modified) llvm/include/llvm/Support/DebugLog.h (+9-3)
  • (modified) mlir/lib/Dialect/GPU/Transforms/EliminateBarriers.cpp (+20-37)
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)
Copy link
Member

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).

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.
@joker-eph joker-eph force-pushed the ldbg_eliminatebarrier branch from 130822e to 9e859fc Compare August 25, 2025 17:22
@joker-eph joker-eph requested a review from jpienaar August 25, 2025 17:30
Copy link
Member

@jpienaar jpienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks

@joker-eph joker-eph merged commit b88e5ca into llvm:main Aug 27, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants