[Mlir-commits] [mlir] [mlir][linalg] Reject unsigned pooling on non-integer element types (PR #166070)

[Abhishek Varma]

================
@@ -579,13 +579,23 @@ class RegionBuilderHelper {
       return arith::MinSIOp::create(builder, arg0.getLoc(), arg0, arg1);
     case BinaryFn::max_unsigned:
       assert(!allComplex);
-      if (allFloatingPoint)
-        return arith::MaximumFOp::create(builder, arg0.getLoc(), arg0, arg1);
+      if (!allInteger || allBool) {
+        if (emitError) {
+          emitError() << "unsupported operation: unsigned max not on uint";
+          return nullptr;
+        }
+        llvm_unreachable("unsupported operation: unsigned max not on uint");
+      }
----------------
Abhishek-Varma wrote:

Hi @Men-cotton - thanks for taking this up.
Although this addresses the immediate issue of `linalg.*_(max|min)_unsigned_*` - a better solution might be to indeed fix :-

https://github.com/llvm/llvm-project/blob/f5885de2cd49785d666b909612e4ec18925abc5a/mlir/python/mlir/dialects/linalg/opdsl/lang/emitter.py#L534-L539

Because that'd ensure that in future some other op implementing a `(max|min)_unsigned` also doesn't warrant a similar fix in their verifier as the one above.

I also see something similar already done in this file for a non-Convolution op : `linalg.div_unsigned` :-
https://github.com/llvm/llvm-project/blob/03ef5fc57064198c3aa4424a722077ab94fbbda5/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp#L561-L568

Again, these are just my opinion - I'll let @banach-space take a closer look. :)

https://github.com/llvm/llvm-project/pull/166070