[PATCH] D73848: Add AffineMaxOp

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Almost there, a couple more comments.



================
Comment at: .gitignore:48
 /.idea
-
 #==============================================================================#
----------------
Spurious change, please remove this from the patch.


================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:265-270
   auto lbValues = expandAffineMap(builder, op.getLoc(), op.getLowerBoundMap(),
                                   op.getLowerBoundOperands());
   if (!lbValues)
     return nullptr;
   return buildMinMaxReductionSeq(op.getLoc(), CmpIPredicate::sgt, *lbValues,
                                  builder);
----------------
Please call `lowerAffineMapMax` here, the same way it is done for `lowerAffineUpperBound` and min below.


================
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();
----------------
I think it should be possible to do `op.map()` isntead of `op.getAttr(T::getMapAttrName())`.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1959
+    p << '[' << operands.drop_front(numDims) << ']';
+  p.printOptionalAttrDict(op.getAttrs(), /*elidedAttrs=*/{"map"});
+}
----------------
Use `T::getMapAttrName()` instead of hardcoded `"map"`.


================
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();
----------------
Nit: update the comment to reflect what the code below does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73848