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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:335
+    
+    mlir::Block *getBody();
+    mlir::OpBuilder getBodyBuilder();
----------------
Drop all of the llvm:: and mlir::


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2169
+    SmallVector<AffineExpr, 8> ubExprs;
+    // Make range expressions for each range
+    for (int64_t range : ranges) {
----------------
nit: I would restructure this given that all of the lb elements are the same.
```
SmallVector<AffineExpr, 8> ubExprs;
for (int64_t range : ranges)
  ubExprs.push_back(builder->getAffineConstantExpr(range));

SmallVector<AffineExpr, 8> lbExprs(ubExprs.size(), builder->getAffineConstantExpr(0));
```



================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2177
+  }
+  // Fall through
+  build(builder, result, lbMap, {}, ubMap, {});
----------------
nit: Can you change all of these comments to be complete sentences?


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2201
+  assert(dims == steps.size());
+  // Set all of the attributes
+  result.addAttribute(getLowerBoundsMapAttrName(), AffineMapAttr::get(lbMap));
----------------
Missing punctuation on each of these. Also please make them complete sentences.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2211
+  // Add all the args
+  for (unsigned i = 0; i < dims; ++i) {
+    body->addArgument(IndexType::get(builder->getContext()));
----------------
Remove the trivial braces.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2215
+  bodyRegion->push_back(body);
+  // Terminate
+  ensureTerminator(*bodyRegion, *builder, result.location);
----------------
This is unnecessary.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2243
+    auto expr = rangesValueMap.getResult(i);
+    if (auto cst = expr.dyn_cast<mlir::AffineConstantExpr>()) {
+      out.push_back(cst.getValue());
----------------
Remove trivial braces.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2243
+    auto expr = rangesValueMap.getResult(i);
+    if (auto cst = expr.dyn_cast<mlir::AffineConstantExpr>()) {
+      out.push_back(cst.getValue());
----------------
rriddle wrote:
> Remove trivial braces.
Can you change this to early exit instead?


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2255
+mlir::OpBuilder AffineParallelOp::getBodyBuilder() {
+  return mlir::OpBuilder(getBody(), std::prev(getBody()->end()));
+}
----------------
Remove all of the mlir:: and llvm::.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2286
+    auto step = attr.cast<IntegerAttr>().getInt();
+    if (step != 1) {
+      elideSteps = false;
----------------
Remove trivial braces. Also, you can just do something like:

```
elideSteps &= (step == 1);
```


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2316
+  SmallVector<OpAsmParser::OperandType, 4> ivs, lowerBoundsMapOperands,
+      upperBoundsMapOperands;
+  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
----------------
nit: This is flowing to the next line, so just split it off and repeat the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74288