[PATCH] D73848: Add AffineMaxOp

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
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);
----------------
OuHangKresnik wrote:
> ftynse wrote:
> > 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)
> Ok, I will do this change in next commit?
Yes please.


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73848