This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Move linalg.fill folding into linalg.generic pattern to elementwise op fusion
ClosedPublic

Authored by nirvedhmeshram on Mar 31 2022, 12:40 PM.

Details

Summary

Move linalg.fill folding into linalg.generic pattern from canonicalization to elementwise op fusion

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 12:40 PM
nirvedhmeshram requested review of this revision.Mar 31 2022, 12:40 PM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
2217–2218

nit: adding a blank line before comments usually helps readability.

2236

[optional] I'd prefer not having this comment. The below line already describes it.

2285–2286

Can merge this into one line?

mlir/test/Dialect/Linalg/fusion-elementwise-ops.mlir
1

Why do we need canoincalize here?

I found that they are copied from canonicalize.mlir. So I assume that there are at least two patterns kicked in to get the result IR. Can we split tests to test two patterns separately?

nirvedhmeshram added inline comments.Mar 31 2022, 2:21 PM
mlir/test/Dialect/Linalg/fusion-elementwise-ops.mlir
1

Yes, so the other pattern it needs is this one
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp#L859
And that pattern has its own tests here that are not being moved by this change
https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/Linalg/canonicalize.mlir#L405-L443

Coming back to the tests and the pattern I am moving here, without the deadarg removing pattern it will have dead args and fill ops with no consumers. I can account for that in the pattern matching checks. However, it is also important to check if the combination works as that is the end goal here. Perhaps one solution is to add a separate test file for checking this combination. What do you think?

hanchung added inline comments.Mar 31 2022, 2:53 PM
mlir/test/Dialect/Linalg/fusion-elementwise-ops.mlir
1

In this case, we can put the output of this pattern to canonicalize.mlir and test it there. I don't have strong opinion about having canonicalize in this file or not. I was afraid that we're not testing what the pattern does. I was also afraid if the test is robust, i.e., if adding other patterns would break this.

After reading patterns deeply, I think it's a fine change to me.

1

I'll let you make the call.

Addressing reviewer comments

nirvedhmeshram marked 6 inline comments as done.Apr 1 2022, 7:46 AM
nirvedhmeshram added inline comments.
mlir/test/Dialect/Linalg/fusion-elementwise-ops.mlir
1

Sounds good, I think we can leave the canonicalize in then.

mravishankar requested changes to this revision.Apr 1 2022, 10:13 AM

Minor change to not use canonicalize in lit test. Otherwise LGTM! THanks!

mlir/test/Dialect/Linalg/fusion-elementwise-ops.mlir
1

I think MLIR convention is to not rely on canonicalize to clean up lit tests. Its pretty disruptive cause changes to canonicalize then hits many lit tests. So It is probably better to remove the canonicalize and test the pass by itself....

This revision now requires changes to proceed.Apr 1 2022, 10:13 AM
nirvedhmeshram marked an inline comment as done.

Addressing reviewer comments and rebase

nirvedhmeshram marked an inline comment as done.Apr 4 2022, 8:16 AM
nirvedhmeshram added a subscriber: benvanik.

There was a bug that was found by @benvanik in the folding pattern where it would trigger infinitely if the fill already did not have a use in the generic op.
Fixing that in this revision. With this change also dead args get dropped automatically with the transform pattern because it calls canonicalization within the pass. The fact that this was not happening initially was due to this bug.

mravishankar requested changes to this revision.Apr 4 2022, 10:46 AM

Minot nits, but thanks for moving it!

mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
2234

Nit: LLVM prefers early exit style.

So

auto fillOp = ...
if (!fillOp) continue;
``
2235

You could just use

if (!genericOp.payloadUsesValueFromOperand(opOperand))
  continue;
This revision now requires changes to proceed.Apr 4 2022, 10:46 AM
nirvedhmeshram marked 2 inline comments as done.
mravishankar accepted this revision.Apr 5 2022, 9:59 AM
This revision is now accepted and ready to land.Apr 5 2022, 9:59 AM