This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Transforms] GreedyPatternRewriteDriver debugging: Detect faulty patterns
ClosedPublic

Authored by springerm on Feb 22 2023, 3:54 AM.

Details

Summary

Compute operation finger prints to detect incorrect API usage in RewritePatterns. Does not work for dialect conversion patterns.

Detect patterns that:

  • Returned failure but changed the IR.
  • Returned success but did not change the IR.
  • Inserted/removed/modified ops, bypassing the rewriter. Not all cases are detected.

These new checks are quite expensive, so they are only enabled with -DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON.

Depends On: D151207

Diff Detail

Event Timeline

springerm created this revision.Feb 22 2023, 3:54 AM
springerm requested review of this revision.Feb 22 2023, 3:54 AM

add config flag

springerm retitled this revision from [mlir][WIP] GreedyPatternRewriteDriver debugging: Detect faulty patterns to [mlir][Transforms] GreedyPatternRewriteDriver debugging: Detect faulty patterns.Apr 28 2023, 12:21 AM
springerm edited the summary of this revision. (Show Details)

Note: There are two more tests that I have to fix before landing this: IR/greedy-pattern-rewriter-driver.mlir and Transforms/test-strict-pattern-driver.mlir.

springerm added a reviewer: ftynse.
jpienaar added inline comments.Apr 28 2023, 4:47 AM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
316

This feels like it could utilize the Action framework work here.

443

Couldn't this be in a listener rather than interspersed here?

mehdi_amini added inline comments.Apr 28 2023, 9:41 AM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
317

I'm not fond of the #if / #else here, and the actual code "doing the work" is now duplicated three times because of nested #if !

I would try to:

  1. outline the debug code (keep the working code "small").
  2. Not duplicate the code that is executed in both debug and non-debug
springerm added inline comments.May 23 2023, 1:26 AM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
316

I'm not sure how to use actions here. I thought about encapsulating all fingerprint-related logic in an action, but that doesn't work because fingerprints must be accessed after a pattern was applied. Also, various listener callbacks (e.g., notifyOperationRemoved) must delete fingerprints.

springerm edited the summary of this revision. (Show Details)May 23 2023, 6:16 AM
springerm updated this revision to Diff 524679.May 23 2023, 6:17 AM
springerm marked 2 inline comments as done.

address comments

mehdi_amini added inline comments.May 23 2023, 5:42 PM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
316

The thing that prevents from using Action I think is that we can't intercept the result of the pattern application I believe, which is important to know if a change in the fingerprint is expected or not.

mehdi_amini accepted this revision.May 23 2023, 5:44 PM
mehdi_amini added inline comments.
mlir/include/mlir/Config/mlir-config.h.cmake
18

I didn't find the NDEBUG requirement by looking at the code?

Also why is ASAN related to this check?

This revision is now accepted and ready to land.May 23 2023, 5:44 PM
springerm marked an inline comment as done.

address comments

mlir/include/mlir/Config/mlir-config.h.cmake
18

NDEBUG is not required indeed. ASAN is recommended because some failures manifest as invalid memory accesses. ASAN shows where the memory was deallocated, which is typically something like op->erase, which should have been rewriter.erase(op). Without ASAN, you just see a crash when the deallocated memory was accessed. I added some extra comments.

mehdi_amini added inline comments.May 24 2023, 12:37 AM
mlir/include/mlir/Config/mlir-config.h.cmake
18

Ah! It's during the fingerprinting: makes sense.

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
74

Actually NDEBUG is required here, but you could just do

if(it.second != OperationFingerPrint(it.first))
   llvm_report_fatal("operation finger print changed");

(same for the other asserts)

springerm updated this revision to Diff 525159.May 24 2023, 6:46 AM
springerm marked an inline comment as done.

address comments

This revision was landed with ongoing or failed builds.May 24 2023, 7:23 AM
This revision was automatically updated to reflect the committed changes.