[PATCH] D74288: [MLIR][Affine] Add affine.parallel op

[Frank Laub via Phabricator via llvm-commits]

flaub marked 2 inline comments as done.
flaub added inline comments.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:288
+    "affine.terminator".  Calling AffineParallelOp::build will create such
+    region and insert the terminator, so will the parsing even in cases if it is
+    absent from the custom format.
----------------
earhart wrote:
> This sentence seems slightly awkward -- maybe something like,
> ```
> Calling AffineParallelOp::build will automatically create the required region and block, and insert the required terminator.  Parsing will also create the required region, block, and terminator, even when they are missing from the textual representation.
> 
> ```
> ?  (Or maybe it's better to keep as-is, for consistency with `AffineForOp`...?)
Re-worded, thanks.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:298
+
+      affine.parallel [%i, %j] = [0, 0] to [10, 10] step [1, 1] {
+        ...
----------------
herhut wrote:
> flaub wrote:
> > jbruestle wrote:
> > > Why use square brackets rather than parens?  In general, tuples are usually parens.
> > This stems from the usage of `parseAffineMapOfSSAIds`, it apparently assumes square bracket delimiters.
> > 
> > @rriddle Would it break things if we allowed `parseAffineMapOfSSAIds` to take a delimiter so that we could group these ids in parens? I'm worried that this might cause ambiguity in the parser.
> I would prefer () here, as well. I do not see an issue with changing the bracket style. Affine expressions should have matching opening/closing brackets, so the parser should not get confused by an extra closing bracket.
I extended the Parser to support specifying the delimiter to use with `parseAffineMapOfSSAIds`, and this works!


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:307
+     I64ArrayAttr:$steps,
+     Variadic<Index>:$mapOperands);
+  let regions = (region SizedRegion<1>:$region);
----------------
herhut wrote:
> You could use `SameVariadicOperandSize` trait and then split the `mapOperands` into upper and lower bound here instead of implementing explicitly.
Great, TIL :)


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2291-2300
+  p << ']';
+  bool elideSteps = true;
+  for (auto attr : op.steps()) {
+    auto step = attr.cast<IntegerAttr>().getValue();
+    if (step != 1) {
+      elideSteps = false;
+      break;
----------------
herhut wrote:
> Maybe `if (llvm::any_of(steps(), ...)) {`
In order to print an array wrapped with parens, I needed to collect the elements of the array anyways. But I'm glad I learned about more LLVM functional helpers.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2343
+  SmallVector<OpAsmParser::OperandType, 4> stepsMapOperands;
+  if (parser.parseOptionalKeyword("step")) {
+    SmallVector<int64_t, 4> steps(ivs.size(), 1);
----------------
herhut wrote:
> Maybe `if (failed(parser...` to make it more obvious that the first one is the failure case?
SGTM


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2348-2364
+    if (parser.parseAffineMapOfSSAIds(stepsMapOperands, stepsMapAttr,
+                                      AffineParallelOp::getStepsAttrName(),
+                                      stepsAttrs))
+      return failure();
+
+    // Convert steps from an AffineMap into an I64ArrayAttr.
+    SmallVector<int64_t, 4> steps;
----------------
herhut wrote:
> Could this code just parse a list of constant integers directly?
The only other Parser method available that I could find suitable was `parseAttribute` into an `ArrayAttr`, however this expects square brackets.

Should we extend the Parser to support this or perhaps save it for another (NFC) day?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74288