[PATCH] D122086: [mlir:PDL] Expand how native constraint/rewrite functions can be defined

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.
This revision is now accepted and ready to land.

Very cool!
The template-fu is strong with this one ..



================
Comment at: mlir/include/mlir/IR/PatternMatch.h:390
+//===----------------------------------------------------------------------===//
+// RewriterBase
+//===----------------------------------------------------------------------===//
----------------
FMI, why was all this code moved?
Is it just for lexicographic order or something else (e.g. avoid forward declarations) ?


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:887
+              PDLValue pdlValue, size_t argIdx) {
+    // Verify the base class before we continuing.
+    if (failed(ProcessPDLValue<BaseT>::verifyAsArg(errorFn, pdlValue, argIdx)))
----------------
nit: grammo


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:910
+/// This struct provides a simplified model for processing types that have
+/// "builtin" PDLValue support.
+template <typename T>
----------------
Could be useful to list them in the doc specifically: `e.g. Attribute, Operation*, Type, Value, TypeRange, ValueRange`


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:931
+/// This struct provides a simplified model for processing types that inherit
+/// from builtin PDLValue types.
+template <typename T, typename BaseT>
----------------
I am not clear what these types are, could you add 1-2 examples in the doc?


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:1091
+
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  auto errorFn = [&](const Twine &msg) -> LogicalResult {
----------------
Hmm .. can you comment on this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122086/new/

https://reviews.llvm.org/D122086