[PSA] Deprecating cast/isa methods in some classes

MLIR Deprecation & Important Refactoring
1

[PSA] Deprecating {Attribute,Type,Value}::{cast,isa,isa_and_nonnull,dyn_cast,dyn_cast_or_null} methods

Context

As noted on the deprecation page, Deprecations & Current Refactoring - MLIR, and originally discussed on Discourse, Preferred casting style going forward, free function isa/cast/… functionality is preferred to method calls. Concretely:

// Preferred
llvm::cast<Value>(*this);

// Deprecated
this->cast<Value>();

At this point, Attribute, Type, Value, Op, Location, and PointerUnion classes and subclasses supported both function and method based casts. I have created a clang-tidy check to provide updating of most call sites of these methods to be function calls, and with some manual changes MLIR can mark these methods as deprecated.

One can see what these changes look like in the commits listed here: Commits · llvm/llvm-project · GitHub

Request

Please comment if there are objections to this change, objections to how this is being handled, or requests for how to approach it. Otherwise, I hope to mark the methods in MLIR as deprecated next week and to remove the methods 1-2 months later.

As an example of a potentially valid concern, this change will result in mixed call styles because now some classes are updated to use function calls, while others only support method calls, so please comment if you consider this a blocking concern.

Below, one will find the tooling to automate a large portion of the changes, but I did the .td file changes manually with find-next + vim-macros

What to Do

The linked branch has 3 commits:

  • 2 commits that provide clang-tidy tooling to handle almost all C++ code. They probably could have been combined, but I didn’t verify for sure, so I left them separately. Running the first commit and second commit one after the other covered all C++ code in MLIR. Running at only the second commit handled all of C++ code in Flang.
  • 1 commit showing the deprecated methods. Due to llvm::TypeSwitch having a call to the dyn_cast method, I did not mark those methods as deprecated.

Note: These commits only handle C++ code that can be analyzed, so C++ code inside .td files and in macros are not automatically updated.

Note: This clang-tidy does some additional changes that are not strictly necessary:

  1. llvm::PointerUnion calls were updated, although this functionality cannot be easily removed after MLIR call sites are updated.
  2. dyn_cast_or_null is replaced with dyn_cast_if_present

Warning: I wanted to begin this announcement, but, assuming no objections, I will update my branch one more time next week at the same time that I announce the deprecation and then update the commands here with the correct hashes.

As an example, updating Flang could be done with:

curl https://github.com/tpopp/llvm-project/compare/fe7afcf70c93223a16ec7a2a5e07c4ace16c9a04...tpopp:llvm-project:f5d2f889fe00d3396494ad8e605bbee47e046d20.patch | git apply
git add -u
git commit -m "temporary clang-tidy patch"
ninja -C $BUILD_DIR  check-flang
ninja -C $BUILD_DIR  clang-tidy
run-clang-tidy -clang-tidy-binary=$BUILD_DIR/bin/clang-tidy -checks='-*,misc-cast-functions'  flang/* -header-filter=flang/ -fix

One will afterwards have to manually update any other code that is missed, such as the mentioned macros and .td files.

What is Incomplete

I did not have time to carry this farther to completion. The process of deprecating Location’s methods is incomplete but should be short and easy when someone handles it. Furthermore, any classes that did not already provide the needed functionality for llvm::cast() style calls are not updated now. An example of this is AffineExpr which only provides the methods.

Bonus Request

If one is an owner of a class, such as AffineExpr, please consider implementing LLVM’s CastInfo for your class, so the usage of cast/isa becomes more consistent. Almost all call sites of your class’ methods should be updated simply by patching my clang-tidy script and adding a call AddMatchers("::mlir::YourClass", "Callee"); before rerunning the clang-tidy process

Thank you

All feedback is welcome
Tres

1 Like