[PATCH] D73848: Add AffineMaxOp

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: .gitignore:48
 /.idea
-
+/third_party
+/release
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:237
 
-def AffineMinOp : Affine_Op<"min"> {
+class Affine_ReduceOp <string mnemonic, list<OpTrait> traits = []> :
+    Op<Affine_Dialect, mnemonic, traits> {
----------------
OuHangKresnik wrote:
> nicolasvasilache wrote:
> > this should be in a separate revision.
> 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 ?
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`.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:237
 
-def AffineMinOp : Affine_Op<"min"> {
+class Affine_ReduceOp <string mnemonic, list<OpTrait> traits = []> :
+    Op<Affine_Dialect, mnemonic, traits> {
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:238
+class Affine_ReduceOp <string mnemonic, list<OpTrait> traits = []> :
+    Op<Affine_Dialect, mnemonic, traits> {
+  let arguments = (ins AffineMapAttr:$map, Variadic<Index>:$operands);
----------------
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)


================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:314
+    Value reduced =
+        lowerAffineMapMin(rewriter, op.getLoc(), op.map(), op.operands());
+    if (!reduced)
----------------
This looks suspicious: why are you calling "lowerAffineMap*Min*` in the pattern for **max** ?


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2027
+
+OpFoldResult AffineMaxOp::fold(ArrayRef<Attribute> operands) {
+  // Fold the affine map.
----------------
OuHangKresnik wrote:
> nicolasvasilache wrote:
> > this should be in a separate revision and have a test in canonicalize.mlir.
> I wrote two tests in canonicalize.mlir which follows the tests of AffineMin,  do you mean more tests needed there ?
I'm fine with this change being in the same revision. It's small enough to review.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1938
 //===----------------------------------------------------------------------===//
-// AffineMinOp
+// AffineReduceOp
 //===----------------------------------------------------------------------===//
----------------
There's no such thing as AffineReduceOp.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1988
 
-static LogicalResult verify(AffineMinOp op) {
-  // Verify that operand count matches affine map dimension and symbol count.
-  if (op.getNumOperands() != op.map().getNumDims() + op.map().getNumSymbols())
-    return op.emitOpError(
-        "operand count and affine map dimension and symbol count must match");
-  return success();
+static ParseResult parseAffineMinOp(OpAsmParser &parser,
+                                    OperationState &result) {
----------------
You can avoid this function by using something like `return parseAffineReduceOp<$cppClass>(parser, result);` in the ODS file.


================
Comment at: mlir/test/AffineOps/ops.mlir:84
+func @affine_max(%arg0 : index, %arg1 : index, %arg2 : index) {
+  // CHECK: affine.max #[[MAP0]](%arg0)[%arg1]
+  %0 = affine.max affine_map<(d0)[s0] -> (1000, d0 + 512, s0)> (%arg0)[%arg1]
----------------
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.


================
Comment at: mlir/test/Transforms/lower-affine.mlir:618-619
+  // CHECK: %[[second:.*]] = addi %[[ARG1]], %[[neg2]]
+  // CHECK: %[[cmp:.*]] = cmpi "slt", %[[first]], %[[second]]
+  // CHECK: select %[[cmp]], %[[first]], %[[second]]
+  %0 = affine.max affine_map<(d0,d1) -> (d0 - d1, d1 - d0)>(%arg0, %arg1)
----------------
This lowering looks like it computes min, not max.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73848/new/

https://reviews.llvm.org/D73848