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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:390
+//===----------------------------------------------------------------------===//
+// RewriterBase
+//===----------------------------------------------------------------------===//
----------------
nicolasvasilache wrote:
> FMI, why was all this code moved?
> Is it just for lexicographic order or something else (e.g. avoid forward declarations) ?
It allows for the templates to invoke some methods on the rewriter for convenience.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:821
+///
+///  template <typename T>
+/// void foo() {
----------------
jpienaar wrote:
> Micro nit: extra space :)
> 
> Doesn't some of these also go away with C++17? (I could be confused)
Not that I know of, we always need the false to be dependent on the template type, otherwise it asserts in contexts we don't want it to.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:979
+    static_assert(always_false<T>,
+                  "Prefer using StringRef for string-like arguments");
+  }
----------------
jpienaar wrote:
> Is this only a preference or a requirement given what is stored?
It's not an inherent "hard" requirement, but it would involve copying the string (which is a waste). Reworded the message a bit.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:1091
+
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  auto errorFn = [&](const Twine &msg) -> LogicalResult {
----------------
nicolasvasilache wrote:
> Hmm .. can you comment on this?
It's essentially just `#ifndef NDEBUG`


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:1128
+    (void)std::initializer_list<int>{
+        0, (processResults(rewriter, results, std::move(args)), 0)...};
+  };
----------------
jpienaar wrote:
> I don't quite follow the 0's here (I may just not be following the logic at all, could you add short explanation?)
It's a fold expression. In C++14 you can only do it in certain situations, e.g. initializer_lists. The zeros are just to satisfy having an `int` expression for the initializer_list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122086