Page MenuHomePhabricator

[MLIR] Simplify CmpIOp when operands are identical
AcceptedPublic

Authored by gtbercea on Jul 30 2020, 4:02 PM.

Details

Summary

This patch simplifies the cmpi Standard dialect operation when its operands are identical and it replaces it with a constant representing the outcome of the comparison.

Diff Detail

Event Timeline

gtbercea created this revision.Jul 30 2020, 4:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 4:02 PM
gtbercea requested review of this revision.Jul 30 2020, 4:02 PM
rriddle added inline comments.Jul 30 2020, 4:36 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
878

This looks like it should be part of the fold method instead.

887

Can you use proper operand accessors instead of this? (.lhs()/.rhs())

mlir/test/Dialect/Standard/canonicalize.mlir
14

Please avoid the use of SSA value names inside of check lines, use captures instead.

gtbercea updated this revision to Diff 282220.Jul 31 2020, 7:48 AM
gtbercea marked 2 inline comments as done.
gtbercea added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
878

I tried that but it led to many lit test failures. I think the problem is that the input to fold are attributes whereas here I have Value operands.

rriddle requested changes to this revision.Jul 31 2020, 11:47 AM
rriddle added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
878

The input attributes to fold are operands that are known to be constant, i.e. you can still use the operands directly from the operation. If you are getting lit failures, that is due to something else. (e.g. tests being broken by better folding that changed the expected output)

This revision now requires changes to proceed.Jul 31 2020, 11:47 AM
gtbercea updated this revision to Diff 282309.Jul 31 2020, 1:55 PM
gtbercea added inline comments.Jul 31 2020, 1:58 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
878

Good point, I completely ignored the fact that I could get the operands as usual even when inside the fold function! It all works now. :)

rriddle added inline comments.Jul 31 2020, 2:37 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
860

nit: Can you use BoolAttr::get here instead? BoolAttr is a utility wrapper around i1 IntegerAttr.

mlir/test/Dialect/Standard/canonicalize.mlir
8

Can you trim out all of the unnecessary operations here? You should be able to test these cases with just the cmpi and function arguments.

25

Can you make sure to add tests for each of the other cases that you are folding?

gtbercea updated this revision to Diff 282333.Jul 31 2020, 3:22 PM
rriddle added inline comments.Jul 31 2020, 3:24 PM
mlir/test/Dialect/Standard/canonicalize.mlir
4

Do you need the select in any of these? Why not return the comparison result directly?

gtbercea updated this revision to Diff 282345.Jul 31 2020, 4:04 PM
rriddle accepted this revision.Jul 31 2020, 4:08 PM

Thanks. LGTM after resolving the last few comments.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
849

This isn't possible, the operands to an operation are always valid. You only have to check null when accessing attributes from the operands parameter, as a null Attribute means that the operand isn't constant.

mlir/test/Dialect/Standard/canonicalize.mlir
7

nit: The general style is to have this before not after.

8

nit: These CHECK-NOTS can be simplified to just CHECK-NOT: cmpi

This revision is now accepted and ready to land.Jul 31 2020, 4:08 PM
gtbercea updated this revision to Diff 282458.Aug 2 2020, 9:58 AM
gtbercea marked 5 inline comments as done.Aug 11 2020, 9:50 AM
gtbercea marked 3 inline comments as done.

Would someone be able to commit this? Thanks a lot.