[Mlir-commits] [mlir] [mlir] Cleanup dump() functions. (PR #89216)

[Michael Kruse]

================
@@ -1019,16 +1019,12 @@ class alignas(8) Operation final
 
   /// Expose a few methods explicitly for the debugger to call for
   /// visualization.
-#ifndef NDEBUG
-  LLVM_DUMP_METHOD operand_range debug_getOperands() { return getOperands(); }
-  LLVM_DUMP_METHOD result_range debug_getResults() { return getResults(); }
-  LLVM_DUMP_METHOD SuccessorRange debug_getSuccessors() {
-    return getSuccessors();
-  }
-  LLVM_DUMP_METHOD MutableArrayRef<Region> debug_getRegions() {
-    return getRegions();
-  }
-#endif
+  /// @{
+  operand_range debug_getOperands();
----------------
Meinersbur wrote:

So the FIXME was fixed by llvm-config.h?

However, the `LLVM_DUMP_METHOD` makes use of the `NDEBUG` macro which is under the including's application's control and `LLVM_ENABLE_DEBUG` is not present in `llvm-config.h`.


> This makes it possible to turn a compile-time failure into a link-time failure, that seems like a "regression" to me in behavior.

This is by design of the `LLVM_DUMP_METHOD` pattern established in 8c209aa8779de57771b64896f805e14cc0016dcd. You were not supposed to call `debug_getOperands()` in code, just from the debugger, so if you had a compile-time failure, you were using it wrong. I think there wouldn't be a lot of bloat if we also had dump functions in release builds, but I think such a change should be LLVM-wise.

I guess one could make some dump functions be including-application controlled by user application macros. However, the cost is high: because by design there is are no uses of such functions, `__attribute__((__used__))` must be used which causes it to be emitted in every single translation unit. 

Anyway, there should be a correct use of the `LLVM_DUMP_METHOD` macro, and MLIR currently doesn't follow any pattern.

https://github.com/llvm/llvm-project/pull/89216