Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

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



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

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


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

mehdi_amini added inline comments.Apr 28 2023, 9:41 AM

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

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

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.

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


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

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


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.