[PATCH] D73848: Add AffineMaxOp
Alex Zinenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 3 02:08:42 PST 2020
ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.
Thanks! I have a couple of comments inline. Major comments:
- the lowering looks incorrect;
- please call `SomethingOp` only the thing that is an actual Op in the IR, use `OpBase` or `Base` for common implementation details.
================
Comment at: .gitignore:48
/.idea
-
+/third_party
+/release
----------------
This should be a separate patch if you intend all of LLVM to follow this. Check out https://stackoverflow.com/questions/1753070/how-do-i-configure-git-to-ignore-some-files-locally if you want to make git ignore files without pushing your personal config upstream.
================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:237
-def AffineMinOp : Affine_Op<"min"> {
+class Affine_ReduceOp <string mnemonic, list<OpTrait> traits = []> :
+ Op<Affine_Dialect, mnemonic, traits> {
----------------
OuHangKresnik wrote:
> nicolasvasilache wrote:
> > this should be in a separate revision.
> Since there are many common code between AffineMax and AffineMIn.
> The introduction of reduceOp (class not def) here is to try to avoid repeated code.
>
> I think the same code could be applied to Ops like avg / sum ops
>
> Any better way ? If there is no such ReduceOp ?
Like I mentioned above, calling it an Op is misleading. But factoring out common code is the right approach.
I don't think we need `affine.add`, that can be perfectly expressed with `affine.apply` instead, unlike `min` and `max`.
================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:237
-def AffineMinOp : Affine_Op<"min"> {
+class Affine_ReduceOp <string mnemonic, list<OpTrait> traits = []> :
+ Op<Affine_Dialect, mnemonic, traits> {
----------------
This should not be called "Op", it sounds like it is a separate operation but it is not. I would suggest `AffineMinMaxOpBase`, a bit ugly but crystal clear.
================
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);
----------------
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)
================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:314
+ Value reduced =
+ lowerAffineMapMin(rewriter, op.getLoc(), op.map(), op.operands());
+ if (!reduced)
----------------
This looks suspicious: why are you calling "lowerAffineMap*Min*` in the pattern for **max** ?
================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2027
+
+OpFoldResult AffineMaxOp::fold(ArrayRef<Attribute> operands) {
+ // Fold the affine map.
----------------
OuHangKresnik wrote:
> nicolasvasilache wrote:
> > this should be in a separate revision and have a test in canonicalize.mlir.
> I wrote two tests in canonicalize.mlir which follows the tests of AffineMin, do you mean more tests needed there ?
I'm fine with this change being in the same revision. It's small enough to review.
================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1938
//===----------------------------------------------------------------------===//
-// AffineMinOp
+// AffineReduceOp
//===----------------------------------------------------------------------===//
----------------
There's no such thing as AffineReduceOp.
================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:1988
-static LogicalResult verify(AffineMinOp op) {
- // Verify that operand count matches affine map dimension and symbol count.
- if (op.getNumOperands() != op.map().getNumDims() + op.map().getNumSymbols())
- return op.emitOpError(
- "operand count and affine map dimension and symbol count must match");
- return success();
+static ParseResult parseAffineMinOp(OpAsmParser &parser,
+ OperationState &result) {
----------------
You can avoid this function by using something like `return parseAffineReduceOp<$cppClass>(parser, result);` in the ODS file.
================
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]
----------------
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.
================
Comment at: mlir/test/Transforms/lower-affine.mlir:618-619
+ // CHECK: %[[second:.*]] = addi %[[ARG1]], %[[neg2]]
+ // CHECK: %[[cmp:.*]] = cmpi "slt", %[[first]], %[[second]]
+ // CHECK: select %[[cmp]], %[[first]], %[[second]]
+ %0 = affine.max affine_map<(d0,d1) -> (d0 - d1, d1 - d0)>(%arg0, %arg1)
----------------
This lowering looks like it computes min, not max.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73848/new/
https://reviews.llvm.org/D73848
More information about the llvm-commits
mailing list