[PATCH] D73848: Add AffineMaxOp

[ouhang via Phabricator via llvm-commits]

OuHangKresnik added a comment.

Reply with fixes and several questions



================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1952
+static void printAffineMinMaxOp(OpAsmPrinter &p, T op) {
+  p << op.getOperationName() << ' ' << op.getAttr(T::getMapAttrName());
+  auto operands = op.getOperands();
----------------
ftynse wrote:
> I think it should be possible to do `op.map()` isntead of `op.getAttr(T::getMapAttrName())`.
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"


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2026
+
+  // Compute and return min of folded map results.
+  int64_t max = std::numeric_limits<int64_t>::min();
----------------
ftynse wrote:
> Nit: update the comment to reflect what the code below does.
Thank you, I will be more careful next time


================
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]
----------------
ftynse wrote:
> OuHangKresnik wrote:
> > ftynse wrote:
> > > 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.
> > Here follows the affine_min test.
> > Change them both ? or I just change affine_max
> 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.
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73848