[PATCH] D73848: Add AffineMaxOp

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.


================
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();
----------------
OuHangKresnik wrote:
> 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"
It would be nice to understand _why_, but I'm fine with the current approach.


================
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();
----------------
OuHangKresnik wrote:
> ftynse wrote:
> > Nit: update the comment to reflect what the code below does.
> Thank you, I will be more careful next time
It's generally better not to copy-paste ;)


================
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]
----------------
OuHangKresnik wrote:
> 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
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73848