Page MenuHomePhabricator

[mlir] Add min/max operations to Standard.
ClosedPublic

Authored by pifon2a on Sep 27 2021, 5:47 AM.

Details

Summary

RFC: Add min/max ops

I was following the naming style for Arith dialect in
https://reviews.llvm.org/D110200,
i.e. similar to DivSIOp and DivUIOp I defined MaxSIOp, MaxUIOp.

When Arith PR is landed, I will migrate these ops as well.

Diff Detail

Event Timeline

pifon2a created this revision.Sep 27 2021, 5:47 AM
pifon2a requested review of this revision.Sep 27 2021, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 5:47 AM
pifon2a updated this revision to Diff 375243.Sep 27 2021, 6:48 AM

Updated documentation

mehdi_amini added inline comments.Sep 27 2021, 8:58 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1260

Wrong copy/paste here.

1333

ditto

aartbik added inline comments.Sep 27 2021, 10:41 AM
mlir/docs/Rationale/Rationale.md
346–347

Did you keep this paragraph on purpose. The however, reflects back on a now deleted part?

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1263

this can be worked out a bit better

1336

same

1361

smaller

1385

samller

pifon2a updated this revision to Diff 375359.Sep 27 2021, 12:16 PM
pifon2a marked 7 inline comments as done.

Addressed the comments. Thank you Mehdi and Aart!

mlir/docs/Rationale/Rationale.md
346–347

Thanks for noticing

One last typo

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1336

maximum -> minimum

pifon2a updated this revision to Diff 375369.Sep 27 2021, 12:33 PM

Last typo

pifon2a marked an inline comment as done.Sep 27 2021, 12:33 PM
aartbik added inline comments.Sep 27 2021, 1:19 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1264

Just curious, the NaN are actually anything in the range 0x7f800001 and up. Does the comparison deal with all these correctly? Also, perhaps mention Float.NEGATIVE_INFINITY and Float.POSITIVE_INFINITY as other interesting "corner" values.

mlir/test/Dialect/Standard/expand-ops.mlir
150

looks like you forgot the CHECKs here

aartbik added inline comments.Sep 27 2021, 1:20 PM
mlir/test/Dialect/Standard/expand-ops.mlir
150

also, was the intention to have a maxf_vector as well?

Thanks, Alex! LGTM.
Could you please add some tests to Dialect/Standard/ops.mlir and Dialect/Standard/invalid.mlir.

pifon2a marked 3 inline comments as done.Sep 28 2021, 12:24 AM
pifon2a added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1264

We are not comparing lhs or rhs directly to NaN, we use cmpf 'uno' instead. It should handle these correctly. I don't think that +/- inf are interesting cases for maximum/minimum.

mlir/test/Dialect/Standard/expand-ops.mlir
150

do you mean minf_vector? I don't think it would test anything that was not tested in maxf_vector test.

pifon2a updated this revision to Diff 375482.Sep 28 2021, 12:24 AM
pifon2a marked 2 inline comments as done.

Address the comments.

Thanks, Alex! LGTM.
Could you please add some tests to Dialect/Standard/ops.mlir and Dialect/Standard/invalid.mlir.

I added the tests to ops.mlir. I am not sure what we would test in invalid.mlir, since the ops are just standard binary ops that were tested multiple times.

This revision is now accepted and ready to land.Sep 28 2021, 12:39 AM
This revision was automatically updated to reflect the committed changes.