This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix unsafe create operation in GreedyPatternRewriter
ClosedPublic

Authored by mravishankar on Mar 18 2020, 11:28 PM.

Details

Summary

When trying to fold an operation during operation creation check that
the operation folding succeeds before inserting the op.

Diff Detail

Event Timeline

mravishankar created this revision.Mar 18 2020, 11:28 PM
rriddle accepted this revision.Mar 19 2020, 12:09 AM
rriddle added inline comments.
mlir/include/mlir/Transforms/FoldUtils.h
78–87

Can you add comments as to why we don't use builder.create directly?

83

You will also want to insert in the case of successful fold and getNumResults() == 0. In that case it is treated as an in-place fold of a zero-result operation.

This revision is now accepted and ready to land.Mar 19 2020, 12:09 AM
mehdi_amini added inline comments.Mar 19 2020, 12:58 AM
mlir/include/mlir/Transforms/FoldUtils.h
91

I think that is similar to what River mentions above, but this code is fishy: if none of the branch is taken the op just leaks.

I'd write it with early exit so that there is a clear handling of the "fallback" path.

Addressing comments

mravishankar marked 4 inline comments as done.Mar 19 2020, 9:14 AM
mravishankar added inline comments.
mlir/include/mlir/Transforms/FoldUtils.h
91

I changed it. I think the logic now maps to what River suggested. Something is off though. Test failing (maybe another issue exposed by this). Looking into those.

mravishankar marked an inline comment as done.Mar 19 2020, 9:36 AM
rriddle requested changes to this revision.Mar 19 2020, 11:01 AM
rriddle added inline comments.
mlir/lib/Transforms/Utils/FoldUtils.cpp
168 ↗(On Diff #251415)

Hmm, this is wrong. You can't assume the insertion point is on a valid operation, the block could be empty. You would need to use the block.

This revision now requires changes to proceed.Mar 19 2020, 11:01 AM
rriddle added inline comments.Mar 19 2020, 11:04 AM
mlir/lib/Transforms/Utils/FoldUtils.cpp
168 ↗(On Diff #251415)

Passing in the insertion block instead of a builder would clean a bit of this up.

Addressing comments

mravishankar marked 3 inline comments as done.Mar 19 2020, 4:03 PM
mravishankar added inline comments.
mlir/lib/Transforms/Utils/FoldUtils.cpp
168 ↗(On Diff #251415)

Good point. CHanged.

rriddle accepted this revision.Mar 20 2020, 2:03 PM
This revision is now accepted and ready to land.Mar 20 2020, 2:03 PM
This revision was automatically updated to reflect the committed changes.