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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

Some small comments. With changes syntax this looks great.



================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:298
+
+      affine.parallel [%i, %j] = [0, 0] to [10, 10] step [1, 1] {
+        ...
----------------
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.


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


================
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;
----------------
Maybe `if (llvm::any_of(steps(), ...)) {`


================
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);
----------------
Maybe `if (failed(parser...` to make it more obvious that the first one is the failure case?


================
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;
----------------
Could this code just parse a list of constant integers directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74288