This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Connect Transform dialect to PDL
ClosedPublic

Authored by ftynse on Apr 19 2022, 8:29 AM.

Details

Summary

This introduces a pair of ops to the Transform dialect that connect it to PDL
patterns. Transform dialect relies on PDL for matching the Payload IR ops that
are about to be transformed. For this purpose, it provides a container op for
patterns, a "pdl_match" op and transform interface implementations that call
into the pattern matching infrastructure.

To enable the caching of compiled patterns, this also provides the extension
mechanism for TransformState. Extensions allow one to store additional
information in the TransformState and thus communicate it between different
Transform dialect operations when they are applied. They can be added and
removed when applying transform ops. An extension containing a symbol table in
which the pattern names are resolved and a pattern compilation cache is
introduced as the first client.

Depends On D123664

Diff Detail

Event Timeline

ftynse created this revision.Apr 19 2022, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 8:29 AM
ftynse requested review of this revision.Apr 19 2022, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 8:29 AM

Tranfsorm -> Transform in the summary.

Mogball added inline comments.Apr 19 2022, 11:36 AM
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
188

Document these new functions?

200

Document this trait?

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
356

Ah, here's the documentation. Some of it should be reflected in the TD file (or just point to here).

rriddle added inline comments.Apr 19 2022, 11:48 AM
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
106

Feels a bit backwards to have a constraint function built as a rewrite.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
161

typo: ontained

188

typo: specifeid

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
176–181

Can we statically assert the presence of the OneRegion and SingleBlock traits?

mlir/lib/Dialect/Transform/IR/TransformOps.cpp
214

break after?

ftynse edited the summary of this revision. (Show Details)Apr 19 2022, 12:42 PM
ftynse updated this revision to Diff 423717.Apr 19 2022, 1:15 PM
ftynse marked 6 inline comments as done.

Address review.

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
176–181

This runs into some strange template behavior, possible recursive instantiation that I haven't had time to debug

third_party/llvm/llvm-project/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h:358:22: error: no member named 'hasTrait' in 'Empty'
      OpTy::template hasTrait<OpTrait::OneRegion>(),
      ~~~~~~         ^
third_party/llvm/llvm-project/mlir/include/mlir/Support/TypeID.h:209:45: note: in instantiation of template class 'mlir::transform::PossibleTopLevelTransformOpTrait<Empty>' requested here
  using has_resolve_typeid_trait = decltype(T::resolveTypeID());
                                            ^
third_party/llvm/llvm-project/llvm/include/llvm/ADT/STLExtras.h:84:24: note: in instantiation of template type alias 'has_resolve_typeid_trait' requested here
struct detector<void_t<Op<Args...>>, Op, Args...> {
                       ^
third_party/llvm/llvm-project/llvm/include/llvm/ADT/STLExtras.h:97:1: note: during template argument deduction for class template partial specialization 'detector<void_t<Op<Args...>>, Op, Args...>' [with Op = mlir::detail::InlineTypeIDResolver::has_resolve_typeid_trait, Args = <mlir::transform::PossibleTopLevelTransformOpTrait<Empty>>]
using is_detected = typename detail::detector<void, Op, Args...>::value_t;
^
third_party/llvm/llvm-project/llvm/include/llvm/ADT/STLExtras.h:97:1: note: in instantiation of template class 'llvm::detail::detector<void, mlir::detail::InlineTypeIDResolver::has_resolve_typeid_trait, mlir::transform::PossibleTopLevelTransformOpTrait<Empty>>' requested here
third_party/llvm/llvm-project/mlir/include/mlir/Support/TypeID.h:211:3: note: in instantiation of template type alias 'is_detected' requested here
  using has_resolve_typeid = llvm::is_detected<has_resolve_typeid_trait, T>;
  ^
third_party/llvm/llvm-project/mlir/include/mlir/Support/TypeID.h:223:47: note: (skipping 3 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
    T, std::enable_if_t<InlineTypeIDResolver::has_resolve_typeid<T>::value>> {
                                              ^
third_party/llvm/llvm-project/mlir/include/mlir/Support/TypeID.h:239:18: note: in instantiation of function template specialization 'mlir::TypeID::get<mlir::transform::PossibleTopLevelTransformOpTrait<Empty>>' requested here
  return TypeID::get<Trait<Empty>>();
                 ^
third_party/llvm/llvm-project/mlir/include/mlir/IR/OperationSupport.h:139:29: note: in instantiation of function template specialization 'mlir::TypeID::get<mlir::transform::PossibleTopLevelTransformOpTrait>' requested here
    return hasTrait(TypeID::get<Trait>());
                            ^
third_party/llvm/llvm-project/mlir/include/mlir/IR/Operation.h:473:17: note: in instantiation of function template specialization 'mlir::OperationName::hasTrait<mlir::transform::PossibleTopLevelTransformOpTrait>' requested here
    return name.hasTrait<Trait>();
                ^
third_party/llvm/llvm-project/mlir/include/mlir/IR/Operation.h:136:15: note: in instantiation of function template specialization 'mlir::Operation::hasTrait<mlir::transform::PossibleTopLevelTransformOpTrait>' requested here
      if (op->hasTrait<Trait>())
              ^
third_party/llvm/llvm-project/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp:192:15: note: in instantiation of function template specialization 'mlir::Operation::getParentWithTrait<mlir::transform::PossibleTopLevelTransformOpTrait>' requested here
          op->getParentWithTrait<PossibleTopLevelTransformOpTrait>()) {

Ideas how to fix that are appreciated!

Mogball added inline comments.Apr 19 2022, 3:37 PM
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
115–117
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
87–91
mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
347–348

Why only one? It seems overly restrictive.

mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
18

description?

93–96

This seems like a little too much of a hack for me. We should introduce a pdl.match %0 op that just makes it so that the pattern returns success if the root op matches and failure otherwise and has a no-op/empty/nonexistant rewriter.

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
121

= default

mlir/lib/Dialect/Transform/IR/TransformOps.cpp
40–41

?

96–97

This no-op rewrite function is registered in two places: here and in TransformDialect::initialize. What's the fate of the latter one?

ftynse updated this revision to Diff 423880.Apr 20 2022, 5:43 AM
ftynse marked 7 inline comments as done.

Address review.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
347–348

Because there's only one Payload IR root op handle that we can bind when this op is used as top-level (without operands). Adding more operand for cases when it is not top-level is premature generalization, especially because the ops are not isolated from above so they can just use the available values directly.

mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
93–96

I am happy to use that if/when it is introduced.

mlir/lib/Dialect/Transform/IR/TransformOps.cpp
96–97

The other one is unnecessary, I removed it.

ftynse updated this revision to Diff 423882.Apr 20 2022, 5:51 AM

Fix misrebase.

Mogball accepted this revision.Apr 20 2022, 11:19 AM
Mogball added inline comments.
mlir/include/mlir/Dialect/Transform/IR/TransformOps.td
93–96

I took a quick look yesterday and it wouldn't involve much effort. I'll just need to cut out some time to do it.

This revision is now accepted and ready to land.Apr 20 2022, 11:19 AM
ftynse updated this revision to Diff 424152.Apr 21 2022, 4:35 AM

Try fixing Windows.

This revision was automatically updated to reflect the committed changes.

We are seeing an error when building on Ubuntu with warnings treated as errors after this change:

In file included from /mnt/vss/_work/1/llvm-project/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp:9:
/mnt/vss/_work/1/llvm-project/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h:185:21: error: private field 'state' is not used [-Werror,-Wunused-private-field]
    TransformState &state;

Is there a need for this field? If not, we should remove it.

Yes, it will be required. b84f95fe5348ac97d33ffd2f6845b7f1b17b389c should silence the warning.