[PATCH] D78226: [MLIR] Improve support for 0-dimensional Affine Maps.

[Jeremy Bruestle via Phabricator via llvm-commits]

jbruestle created this revision.
jbruestle added reviewers: flaub, bondhugula, rriddle, nicolasvasilache, ftynse, ulysseB.
Herald added subscribers: llvm-commits, frgossen, grosul1, bader, Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, burmako, jpienaar, mehdi_amini.
Herald added a reviewer: mravishankar.
Herald added a reviewer: antiagainst.
Herald added 1 blocking reviewer(s): rriddle.
Herald added a reviewer: aartbik.
Herald added a project: LLVM.
rriddle accepted this revision.
rriddle marked an inline comment as done.
This revision is now accepted and ready to land.
jbruestle updated this revision to Diff 257788.
jbruestle added a comment.
jbruestle updated this revision to Diff 257791.
jbruestle marked 3 inline comments as done.

Fixes from code review


jbruestle added a comment.

Another minor issue found during review



================
Comment at: mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h:34
+  auto mapC = AffineMapAttr::get(AffineMap::get(3, 0, {m, n}, context));
+  auto maps = ArrayAttr::get({mapA, mapB, mapC}, context);
   return indexingMaps == maps;
----------------
Unrelated to this revision, but I don't see why this ArrayAttr needs to be generated.


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:1351
     }
     // The (...) -> () affine map has its own factory method.
+    return AffineMap::get(map.getNumDims() - 1, 0, results, ctx);
----------------
This comment appears obsolete.


================
Comment at: mlir/lib/IR/AffineMap.cpp:130
 inferFromExprList(ArrayRef<AffineExprContainer> exprsList) {
+  assert(exprsList.size() != 0);
+  assert(exprsList[0].size() != 0);
----------------
nit: prefer empty instead of size.


================
Comment at: mlir/lib/IR/Builders.cpp:300
 AffineMap Builder::getDimIdentityMap() {
-  return AffineMap::get(/*dimCount=*/1, /*symbolCount=*/0,
-                        {getAffineDimExpr(0)});
+  return AffineMap::get(/*dimCount=*/1, /*symbolCount=*/0, getAffineDimExpr(0));
 }
----------------
Is this wrapped at 80 characters?


================
Comment at: mlir/lib/Transforms/PipelineDataTransfer.cpp:102
+  auto modTwoMap =
+      AffineMap::get(/*dimCount=*/1, /*symbolCount=*/0, d0.floorDiv(step) % 2);
   auto ivModTwoOp = bInner.create<AffineApplyOp>(forOp.getLoc(), modTwoMap,
----------------
Same here, is this formatted?


Modified AffineMap::get to remove support for the overload which allowed
an ArrayRef of AffineExpr but no context (and gathered the context from a
presumed first entry, resulting in bugs when there were 0 results).

Instead, we support only a ArrayRef and a context, and a version which
takes a single AffineExpr.

Additionally, removed some now needless case logic which previously
special cased which call to AffineMap::get to use.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78226

Files:
  mlir/lib/Dialect/Vector/VectorTransforms.cpp


Index: mlir/lib/Dialect/Vector/VectorTransforms.cpp
===================================================================
--- mlir/lib/Dialect/Vector/VectorTransforms.cpp
+++ mlir/lib/Dialect/Vector/VectorTransforms.cpp
@@ -1348,7 +1348,6 @@
       auto targetExpr = getAffineDimExpr(idx < index ? idx : idx - 1, ctx);
       results.push_back(targetExpr);
     }
-    // The (...) -> () affine map has its own factory method.
     return AffineMap::get(map.getNumDims() - 1, 0, results, ctx);
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78226.257791.patch
Type: text/x-patch
Size: 499 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200415/5e124e37/attachment.bin>