[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 ↗

(On Diff #272095)

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 ↗

(On Diff #272095)

Typo (increasing)

mlir/test/mlir-tblgen/pattern.mlir

364 ↗

(On Diff #272095)

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/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));