Page MenuHomePhabricator

Add AffineMaxOp
ClosedPublic

Authored by OuHangKresnik on Feb 2 2020, 6:53 AM.

Diff Detail

Event Timeline

OuHangKresnik created this revision.Feb 2 2020, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2020, 6:53 AM

Unit tests: pass. 62382 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

nicolasvasilache requested changes to this revision.Feb 2 2020, 7:31 AM

please split out (or rebase, not sure) anything related to ReduceOp

This revision now requires changes to proceed.Feb 2 2020, 7:31 AM
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
237–250

this should be in a separate revision.

mlir/lib/Dialect/AffineOps/AffineOps.cpp
1941–1964

this should be in a separate revision.

2023

this should be in a separate revision and have a test in canonicalize.mlir.

OuHangKresnik marked an inline comment as done.

format

Reply

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
237–250

Since there are many common code between AffineMax and AffineMIn.
The introduction of reduceOp (class not def) here is to try to avoid repeated code.

I think the same code could be applied to Ops like avg / sum ops

Any better way ? If there is no such ReduceOp ?

mlir/lib/Dialect/AffineOps/AffineOps.cpp
2023

I wrote two tests in canonicalize.mlir which follows the tests of AffineMin, do you mean more tests needed there ?

Unit tests: pass. 62382 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse requested changes to this revision.Feb 3 2020, 2:01 AM

Thanks! I have a couple of comments inline. Major comments:

  • the lowering looks incorrect;
  • please call SomethingOp only the thing that is an actual Op in the IR, use OpBase or Base for common implementation details.
.gitignore
47–48

This should be a separate patch if you intend all of LLVM to follow this. Check out https://stackoverflow.com/questions/1753070/how-do-i-configure-git-to-ignore-some-files-locally if you want to make git ignore files without pushing your personal config upstream.

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
237–250

This should not be called "Op", it sounds like it is a separate operation but it is not. I would suggest AffineMinMaxOpBase, a bit ugly but crystal clear.

237–250

Like I mentioned above, calling it an Op is misleading. But factoring out common code is the right approach.

I don't think we need affine.add, that can be perfectly expressed with affine.apply instead, unlike min and max.

238

I think we should mark both min and max as not having side effects (in a follow-up commit since min was not marked as such and the change may break something)

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
323

This looks suspicious: why are you calling "lowerAffineMap*Min*` in the pattern for max ?

mlir/lib/Dialect/AffineOps/AffineOps.cpp
1938

There's no such thing as AffineReduceOp.

1980–1981

You can avoid this function by using something like return parseAffineReduceOp<$cppClass>(parser, result); in the ODS file.

2023

I'm fine with this change being in the same revision. It's small enough to review.

mlir/test/AffineOps/ops.mlir
84

Please don't use SSA names in CHECK lines, even if the file around does. See https://mlir.llvm.org/getting_started/TestingGuide/ for more details.

mlir/test/Transforms/lower-affine.mlir
618–619

This lowering looks like it computes min, not max.

This revision now requires changes to proceed.Feb 3 2020, 2:01 AM
OuHangKresnik marked 9 inline comments as done.

Thank you for the review! Replied with some questions.

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
238

Ok, I will do this change in next commit?

mlir/test/AffineOps/ops.mlir
84

Here follows the affine_min test.
Change them both ? or I just change affine_max

Unit tests: pass. 62414 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse added inline comments.Feb 3 2020, 9:52 AM
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
238

Yes please.

mlir/test/AffineOps/ops.mlir
84

I'm not sure I understand the question. The source IR here is a "max" operation. The code you generate computes the minimum instead. It's incorrect and needs to be fixed. Existing tests are correct.

ftynse added a comment.Feb 4 2020, 1:25 AM

Almost there, a couple more comments.

.gitignore
48

Spurious change, please remove this from the patch.

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
265–270

Please call lowerAffineMapMax here, the same way it is done for lowerAffineUpperBound and min below.

mlir/lib/Dialect/AffineOps/AffineOps.cpp
1952

I think it should be possible to do op.map() isntead of op.getAttr(T::getMapAttrName()).

1959

Use T::getMapAttrName() instead of hardcoded "map".

2026

Nit: update the comment to reflect what the code below does.

OuHangKresnik marked 7 inline comments as done.

Reply with fixes and several questions

mlir/lib/Dialect/AffineOps/AffineOps.cpp
1952

It will cause many test breaks such as :

/disk2/ouhang.oh/llvm-project/mlir/test/Dialect/Linalg/tile.mlir:257:12: error: TILE-2: expected string not found in input
// TILE-2: %[[szM:.*]] = affine.min #[[bound_map]](%[[C2]], %[[localM]], %[[I]])

and

<stdin>:37:18: note: with "MAP0" equal to "map0"

2026

Thank you, I will be more careful next time

mlir/test/AffineOps/ops.mlir
84

Here follows the affine_min test.
Change them both ? or I just change affine_max

I mean should I change affine_max here only or both affine_max and affine_min ?
Or keep them the same here and change both of them in next commit ?

The source IR here is a "max" operation. The code you generate computes the minimum instead

In my current understanding, the test here is correct ?

And I just fixed the lower-affine.mlir, due to misuse of the CmpIPredicate

Unit tests: pass. 62414 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Feb 5 2020, 4:27 AM
ftynse added inline comments.
mlir/lib/Dialect/AffineOps/AffineOps.cpp
1952

It would be nice to understand _why_, but I'm fine with the current approach.

2026

It's generally better not to copy-paste ;)

mlir/test/AffineOps/ops.mlir
84

I mean should I change affine_max here only or both affine_max and affine_min ?
Or keep them the same here and change both of them in next commit ?

I don't see why do you need to change affine_min in any way here. It is generally bad practice to change the tests that are not related to the code changes you are making.

In my current understanding, the test here is correct ?

Looks okay to me.

nicolasvasilache accepted this revision.Feb 5 2020, 10:30 AM
This revision is now accepted and ready to land.Feb 5 2020, 10:30 AM
This revision was automatically updated to reflect the committed changes.