[mlir] Allow move-only RewritePattern arguments

[rriddle]

mlir/include/mlir/IR/PatternMatch.h

923

You should also forward the first arg.

[csigg]

Also forward first argument.

[rriddle]

Are the test failures related? They look like they might be.

[csigg]

In D103591#2803316, @rriddle wrote:

Are the test failures related? They look like they might be.

Yes, they are real. The arguments here are rvalues, so after the first pattern they are use-after-move.

I'm not sure if I want to proceed with this change. It might be safer to require using the add() overload on line 960 if pattern arguments are move-only. WDYT?

[rriddle]

In D103591#2807148, @csigg wrote:

In D103591#2803316, @rriddle wrote:

Are the test failures related? They look like they might be.

Yes, they are real. The arguments here are rvalues, so after the first pattern they are use-after-move.

I'm not sure if I want to proceed with this change. It might be safer to require using the add() overload on line 960 if pattern arguments are move-only. WDYT?

Yeah, I think that is a reasonable trade off. The other thing we could do is have an add<> overload for when there is only one pattern being added. We could keep the multi-pattern add the way it is now, but have the single-pattern one use perfect forwarding.

[csigg]

The other thing we could do is have an add<> overload for when there is only one pattern being added. We could keep the multi-pattern add the way it is now, but have the single-pattern one use perfect forwarding.

Yes, that would be possible. But it only saves the 'std::make_unique' characters. Probably not worth it.

mlir/include/mlir/IR/PatternMatch.h

template <typename... Ts, typename ConstructorArg,
             typename... ConstructorArgs,
             typename = std::enable_if_t<sizeof...(Ts) != 0>>
   RewritePatternSet &add(ConstructorArg &&arg, ConstructorArgs &&... args) {
     // The following expands a call to emplace_back for each of the pattern
     // types 'Ts'. This magic is necessary due to a limitation in the places
     // that a parameter pack can be expanded in c++11.
     // FIXME: In c++17 this can be simplified by using 'fold expressions'.
     (void)std::initializer_list<int>{
-        0, (addImpl<Ts>(/*debugLabels=*/llvm::None, arg, args...), 0)...};
+        0, (addImpl<Ts>(/*debugLabels=*/llvm::None,
+                        std::forward<ConstructorArg>(arg),
+                        std::forward<ConstructorArgs>(args)...),
+            0)...};
     return *this;
   }
   /// An overload of the above `add` method that allows for attaching a set
   /// of debug labels to the attached patterns. This is useful for labeling
   /// groups of patterns that may be shared between multiple different
   /// passes/users.
   template <typename... Ts, typename ConstructorArg,
             typename... ConstructorArgs,
             typename = std::enable_if_t<sizeof...(Ts) != 0>>
   RewritePatternSet &addWithLabel(ArrayRef<StringRef> debugLabels,
                                   ConstructorArg &&arg,
                                   ConstructorArgs &&... args) {
     // The following expands a call to emplace_back for each of the pattern
     // types 'Ts'. This magic is necessary due to a limitation in the places
     // that a parameter pack can be expanded in c++11.
     // FIXME: In c++17 this can be simplified by using 'fold expressions'.
     (void)std::initializer_list<int>{
-        0, (addImpl<Ts>(debugLabels, arg, args...), 0)...};
+        0, (addImpl<Ts>(debugLabels, std::forward<ConstructorArg>(arg),
+                        std::forward<ConstructorArgs>(args)...),
+            0)...};
     return *this;
   }
 
   /// Add an instance of each of the pattern types 'Ts'. Return a reference to
   /// `this` for chaining insertions.
   template <typename... Ts>
   RewritePatternSet &add() {
     (void)std::initializer_list<int>{0, (addImpl<Ts>(), 0)...};