Clang-tidy checks for common MLIR anti-patterns

MLIR
1

I recently noticed the existence of the use-new-mlir-op-builder clang-tidy check, which made me question what other checks would be beneficial for MLIR projects. After a quick brainstorming session, I came up with a list of common issues that I encountered before:

  1. Changes to the IR inside a RewritePattern class that are not done via the PatternRewriter object (e.g. call op->erase() instead of rewriter.eraseOp()). This is a case mentioned in the documentation, but it remains an easy mistake to do. I believe a clang-tidy check could capture such cases, by targeting the relevant API calls. Similarly, in-place modifications that are not wrapped in rewriter.modifyOpInPlace could be reported.
  2. Uses of an operation after erasure (e.g. logging an operation’s location after calling eraseOp or replaceOp). This is also an easy mistake to make without using a sanitizer to detect them.
  3. Casting an object via dyn_cast and directly accessing its content without checking for nullptr first. This could either be replaced by a call to cast, to at least have the assert in debug builds, or an explicit check in the code.
  4. Storing a mutable member in an OperationPass or derived class. This is prone to issues in multi-threaded scenarios. The documentation also mentions this. It also mentions that one should not inspect / modify the state of sibling operations, but that seems difficult to capture with something like a clang-tidy check.
  5. Inspecting sibling operations inside a verifier, at least for some common patterns (e.g. getDefiningOp over an operand). Also mentioned in the developer guide.

Is there already some active work on such items? If not, I have some time to start writing some checks for the most desired patterns. Suggestions on what other patterns could be targeted are also welcome.

2

I am also thinking that some related items to the execution time could be identified via clang-tidy.

For example, creating an attribute or type that gets discarded, as each creation has overhead:

auto attr = mlir::IntegerAttr::get(...);
if (...) {
    attr = mlir::IntegerAttr::get(...);
}

The originally attribute was added to the context’s bump allocator, which is not a cheap operation.

Similarly, multiple consecutive attribute mutations to an operation (e.g. op->setAttr() calls), which implies multiple DictionaryAttr objects being created.

3

I have created a PR with the second item in the list, to detect uses of MLIR operations after erasure: llvm-mlir-use-after-erase. Currently marked as draft, to first see if there is interest in this check. Feedback is welcome!

The check also detected some instances of use-after-erase in the LLVM repo. Submitted the fixes with the logs here.

4

I think this is valuable to have, having spent likely a month of my life total chasing these kinds of errors in rewrite patterns. Note that draft PRs don’t notify reviewers so you are unlikely to get feedback on the PR until you mark it as ready.

5

Thanks @ftynse! Marked the current PR ready for review. If this one gets approved, I can continue with another check from the list above.

6

As the implementor of the builder check, I’m biasedly pro adding more :slight_smile:

One other one I was working on (but got back logged while being back logged) was flagging passing value types via const reference. Mostly it turned out in reviews folks were thinking FooOp was something heavy and tried to avoid copy.

Another one for me is not following the LLVM/MLIR error conventions.

Then your text did make me wonder about a bit more of an expensive one: flagging where folks are looking up a attribute by string name rather than using the generated helper on the Op (where it exists, so not discardable attribute case), but that may be a rather expensive check.