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
260

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

383

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
272

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
260

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
260

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
17 ↗(On Diff #524679)

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
17 ↗(On Diff #524679)

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
17 ↗(On Diff #524679)

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

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

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.