Page MenuHomePhabricator

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

Authored by AlexEichenberger on Thu, Jun 18, 8:14 AM.

Details

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.

Diff Detail

Event Timeline

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.

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.

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

Great.

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.

Added the requested lit tests

rriddle added inline comments.Fri, Jun 19, 11:14 AM
mlir/test/lib/Dialect/Test/TestPatterns.cpp
39

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

jpienaar accepted this revision.Fri, Jun 19, 3:47 PM

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 ?"

This revision is now accepted and ready to land.Fri, Jun 19, 3:47 PM

latest comments addressed

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

AlexEichenberger marked 3 inline comments as done.

now all comments are done.

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

This revision was automatically updated to reflect the committed changes.