[MLIR] Remove TableGen redundant calls to native calls when creating new operations in DRR TableGen files

[AlexEichenberger]

Summary

Currently, the TableGen rewrite generates redundant native calls in MLIR DRR files. This is a problem as some native calls may involve significant computations (e.g. when performing constant propagation where every values in a large tensor is touched).

The pattern was as follow:

if (native-call(args)) tblgen_attrs.emplace_back(rewriter, attribute, native-call(args))

The replacement pattern compute native-call(args) once and then use it both in the if condition and the emplace_back call.

[jpienaar]

I was wondering about naming collisions (only reason I could think of why previously this wasn't done), but I can't see that affecting this, so LGTM.

Only test I could think of here would be to have a rewrite rule in the test dialect where the native call emits info on every invocation, and then in the test file we just verify only 1 notification seen. Could you add such a test? That would catch if we regress here.

[AlexEichenberger]

I added the { and } around the if statement specifically to avoid name conflicts. I don't like assignment in if-conditional, but since it's both legal and machine generated code (not intended for human consumption), I think it's ok.

I don't have a problem adding a test, could someone point me to a previous test of TableGen output? I have only done literal tests that check MLIR output, and here I would need to test the output of TableGen.

[jpienaar]

In D82101#2101514, @AlexEichenberger wrote:

I don't have a problem adding a test, could someone point me to a previous test of TableGen output?

Great.

In D82101#2101514, @AlexEichenberger wrote:

I have only done literal tests that check MLIR output, and here I would need to test the output of TableGen.

There are tests that verify the output (https://github.com/llvm/llvm-project/blob/master/mlir/test/mlir-tblgen/op-operand.td), but here I'd prefer to verify behavior as that is the more important part for me. So a lit test would be perfect. For example, in https://github.com/llvm/llvm-project/commit/e5a43186d38c239380c22c2629dff748677c4bd1#diff-82ab1468200932e8a33bd161370ed6d1 we tested mlir-tblgen behavior by adding a test op & test rewrite pattern and then using something like what is done in either testing memref deps (https://github.com/llvm/llvm-project/blob/master/mlir/test/Transforms/memref-dependence-check.mlir) to verify number of times invoked.

[AlexEichenberger]

Added the requested lit tests

[rriddle]

mlir/test/lib/Dialect/Test/TestPatterns.cpp

39

Please follow proper naming convention, i.e. camelCase for variables.

[jpienaar]

Looks good with River's comment addressed. Let know if you need help submitting.

mlir/test/lib/Dialect/Test/TestPatterns.cpp

39

Typo (increasing)

mlir/test/mlir-tblgen/pattern.mlir

364

Nit: perhaps "Test that natives calls are only called once during rewrites ?"

[AlexEichenberger]

latest comments addressed

[AlexEichenberger]

Addressed latest spelling requests. I do need help to commit changes, did a few sometimes ago, not sure how to do it now.

[AlexEichenberger]

now all comments are done.

[AlexEichenberger]

code is ready to be dropped, since I don't have commit privilege, can you please assist, thanks.

mlir/test/lib/Dialect/Test/TestOps.td

 def OpI : TEST_Op<"op_i">, Arguments<(ins I32)>, Results<(outs)>;
 def : Pat<(OpH $input), (OpI $input)>;
 
 // Test patterns for zero-input op.
 def OpJ : TEST_Op<"op_j">, Arguments<(ins)>, Results<(outs I32)>;
 def OpK : TEST_Op<"op_k">, Arguments<(ins)>, Results<(outs I32)>;
 def : Pat<(OpJ), (OpK)>;
 
+// Test that natives calls are only called once during rewrites.
+def OpM : TEST_Op<"op_m"> {
+  let arguments = (ins I32, OptionalAttr<I32Attr>:$optional_attr);
+  let results = (outs I32);
+}
+// Pattern add the argument plus a increasing static number hidden in
+// OpMTest function. That value is set into the optional argument.
+// That way, we will know if operations is called once or twice.
+def OpMGetNullAttr : NativeCodeCall<"Attribute()">;
+def OpMAttributeIsNull : Constraint<CPred<"! ($_self)">, "Attribute is null">;
+def OpMVal : NativeCodeCall<"OpMTest($_builder, $0)">;
+def : Pat<(OpM $attr, $optAttr), (OpM $attr, (OpMVal $attr) ),
+    [(OpMAttributeIsNull:$optAttr)]>;
+
 // Test `$_` for ignoring op argument match.
 def TestIgnoreArgMatchSrcOp : TEST_Op<"ignore_arg_match_src"> {
   let arguments = (ins
     AnyType:$a, AnyType:$b, AnyType:$c,
     AnyAttr:$d, AnyAttr:$e, AnyAttr:$f);
 }
 def TestIgnoreArgMatchDstOp : TEST_Op<"ignore_arg_match_dst"> {
   let arguments = (ins AnyType:$b, AnyAttr:$f);

mlir/test/lib/Dialect/Test/TestPatterns.cpp

 
 static void handleNoResultOp(PatternRewriter &rewriter,
                              OpSymbolBindingNoResult op) {
   // Turn the no result op to a one-result op.
   rewriter.create<OpSymbolBindingB>(op.getLoc(), op.operand().getType(),
                                     op.operand());
 }
 
+// Test that natives calls are only called once during rewrites.
+// OpM_Test will return Pi, increased by 1 for each subsequent calls.
+// This let us check the number of times OpM_Test was called by inspecting
+// the returned value in the MLIR output.
+static int64_t opMIncreasingValue = 314159265;
+static Attribute OpMTest(PatternRewriter &rewriter, Value val){
+  int64_t i = opMIncreasingValue++;
+  return rewriter.getIntegerAttr(rewriter.getIntegerType(32), i);
+}
+
 namespace {
 #include "TestPatterns.inc"
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Canonicalizer Driver.
 //===----------------------------------------------------------------------===//
 

mlir/test/mlir-tblgen/pattern.mlir

 func @generateVariadicOutputOpInNestedPattern() -> (i32) {
   // CHECK: %[[cnt5_a:.*]], %[[cnt5_b:.*]]:2, %[[cnt5_c:.*]]:2 = "test.mixed_variadic_out3"()
   // CHECK: %[[res:.*]] = "test.mixed_variadic_in3"(%[[cnt5_a]], %[[cnt5_b]]#0, %[[cnt5_b]]#1, %[[cnt5_c]]#0, %[[cnt5_c]]#1)
   // CHECK: return %[[res]]
 
   %0 = "test.one_i32_out"() : () -> (i32)
   return %0 : i32
 }
+
+//===----------------------------------------------------------------------===//
+// Test that natives calls are only called once during rewrites.
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: redundantTest
+func @redundantTest(%arg0: i32) -> i32 {
+  %0 = "test.op_m"(%arg0) : (i32) -> i32
+  // CHECK: "test.op_m"(%arg0) {optional_attr = 314159265 : i32} : (i32) -> i32
+  return %0 : i32
+}

mlir/tools/mlir-tblgen/RewriterGen.cpp

void PatternEmitter::createAggregateLocalVarsForOpArgs(
 
   os.indent(6) << formatv(
       "SmallVector<Value, 4> tblgen_values; (void)tblgen_values;\n");
   os.indent(6) << formatv(
       "SmallVector<NamedAttribute, 4> tblgen_attrs; (void)tblgen_attrs;\n");
 
   for (int argIndex = 0, e = resultOp.getNumArgs(); argIndex < e; ++argIndex) {
     if (resultOp.getArg(argIndex).is<NamedAttribute *>()) {
-      const char *addAttrCmd = "if ({1}) {{"
-                               "  tblgen_attrs.emplace_back(rewriter."
-                               "getIdentifier(\"{0}\"), {1}); }\n";
+      const char *addAttrCmd = "{ if (auto tmpAttr = {1}) "
+                               "tblgen_attrs.emplace_back(rewriter."
+                               "getIdentifier(\"{0}\"), tmpAttr); }\n";
       // The argument in the op definition.
       auto opArgName = resultOp.getArgName(argIndex);
       if (auto subTree = node.getArgAsNestedDag(argIndex)) {
         if (!subTree.isNativeCodeCall())
           PrintFatalError(loc, "only NativeCodeCall allowed in nested dag node "
                                "for creating attribute");
         os.indent(6) << formatv(addAttrCmd, opArgName,
                                 handleReplaceWithNativeCodeCall(subTree));