This is an archive of the discontinued LLVM Phabricator instance.

[mlir-tblgen] Fix failed matching when binds same operand of an op in different depth
ClosedPublic

Authored by Chia-hungDuan on Jul 8 2021, 11:43 PM.

Details

Summary

For example, we will generate incorrect code for the pattern,

def : Pat<((FooOp (FooOp, $a, $b), $b)), (...)>;

We didn't allow $b to be bond twice with same operand of same op.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jul 8 2021, 11:43 PM
Chia-hungDuan requested review of this revision.Jul 8 2021, 11:43 PM

Fix comments and argument spelling

Just did a quick skim/will look more later

mlir/include/mlir/TableGen/Pattern.h
243

Why not just use a pointer to DagNode?

Fix variable typo

Failed upload the change in previous patches.

jpienaar added inline comments.Jul 14 2021, 6:52 AM
mlir/include/mlir/TableGen/Pattern.h
243

Just commenting to ensure you didn't miss this before.

304

bound

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

So this would have generated incorrect code before, what would the generated code have done?

rdzhabarov added inline comments.Jul 14 2021, 10:12 AM
mlir/include/mlir/TableGen/Pattern.h
188

nit: const llvm::DagInit *.

254–257

could you update comments on methods everywhere when it's applicable.

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

I think it would be beneficial to have a link to paste with the generated code before and after.

mlir/test/mlir-tblgen/pattern.mlir
164

comment seems to be slightly different from the actual pattern

def TestNestedSameOpAndSameArgEqualityPattern :
  Pat<(OpN (OpN $_, $x), $x), (replaceWithValue $x)>;

Nit: typo in the title falied

Chia-hungDuan retitled this revision from [mlir-tblgen] Fix falied matching when binds same operand of an op in different depth to [mlir-tblgen] Fix failed matching when binds same operand of an op in different depth.Jul 16 2021, 4:39 AM
Chia-hungDuan marked 3 inline comments as done.

Addressed review comments

Nit: typo in the title falied

Done.

mlir/include/mlir/TableGen/Pattern.h
188

This is supposed to be an opaque pointer identifier, will it be better not to expose the type? Or do you mean let's cast the type on the caller side? I.e., the SymbolInfo will do the casting.

243

Sorry, I was supposed to cover this during our chat.

DagNode/DagLeaf are accessed by value (like if (DagNode argTree = tree.getArgAsNestedDag(i)) ) so we can't use DagNode* as a identifier.

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

Please check,
https://gist.githubusercontent.com/ChiaHungDuan/312cd6e47e0ff17183d1d0dfbf236b37/raw/c629bf7e4fe164646a8d9170fcde14d8ce92eac5/Operand%2520match%2520diff

Brief summary, there's only x being assigned to the 2nd operand, x0 has the default value(which is incorrect) in 'before'

mlir/test/mlir-tblgen/pattern.mlir
164

Sorry, changed the case and forgot to update it.

jpienaar accepted this revision.Jul 16 2021, 8:51 AM
jpienaar added inline comments.
mlir/include/mlir/TableGen/Pattern.h
243

Lets add a comment to this point here. Then it is locally clear why a less specific type is used.

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

That last sentence is an important one to add to description as that makes it clear what was wrong and how it would have failed.

This revision is now accepted and ready to land.Jul 16 2021, 8:51 AM
Chia-hungDuan marked an inline comment as done.

Addressed review comments

Chia-hungDuan marked an inline comment as done.Jul 18 2021, 5:41 PM
rdzhabarov added inline comments.Jul 19 2021, 12:03 PM
mlir/include/mlir/TableGen/Pattern.h
188

nvm. Yeah, i think it's better to have it here, explicit. I somehow overlooked the intent of the method.