[PATCH] D80188: [mlir][Linalg] Avoid using scf.parallel for non-parallel loops in Linalg ops.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.
This revision is now accepted and ready to land.

Thanks for the cleanup, looks nice!
I think you also have an opportunity to fix it completely by also taking the iterator types into account with 3-5 more locs (and a test).
Approving conditioned on that.



================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp:387
+  GenericLoopNestRangeBuilder<LoopTy>(ivs, linalgRanges,
+                                      op.iterator_types().getValue())([&] {
     auto &b = ScopedContext::getBuilderRef();
----------------
mravishankar wrote:
> nicolasvasilache wrote:
> > you also want to apply the interchange vector with `applyPermutationToVector`
> Can you give me more context here. I am not sure why I need to do that. I am just changing the class used. I dont follow why it is needed now, but wasnt there previously?
I do not remember the chronology of when lowering to loops and the permutation argument was added to tiling but in the same way you are fixing the fact that not all loops are parallel after tiling, we also need to take the iterator types into account.

This also points at missing coverage in the test.

A few lines above we have:
```
  if (!options.interchangeVector.empty())
    applyPermutationToVector(loopRanges, options.interchangeVector);
```

We should also have something along the lines of:
```
  auto iteratorTypes = llvm::to_vector<4>(op.iterator_types().cast<ArrayAttr>().getValue());
  if (!options.interchangeVector.empty())
    applyPermutationToVector(iteratorTypes, options.interchangeVector);
```

Could you please add a test that permutes and be sure that say your outer parallel loop becomes a sequential when appropriate?


================
Comment at: mlir/lib/Dialect/Linalg/Utils/Utils.cpp:129
+template <>
+void mlir::linalg::GenerateLoopNest<scf::ParallelOp>::doit(
+    MutableArrayRef<Value> allIvs, ArrayRef<SubViewOp::Range> loopRanges,
----------------
A few interspersed comments would be welcome on the fact that this recursive impl terminates and where it supports interleaved bands of loop types. 


================
Comment at: mlir/lib/Dialect/Linalg/Utils/Utils.cpp:136
+                         .take_while([](Attribute attr) {
+                           return attr.cast<StringAttr>().getValue() ==
+                                  getParallelIteratorTypeName();
----------------
seems like we could have an `isParallelAttr` in the Utils.h and this would become `xxx.take_while(isParallelAttr)`.


================
Comment at: mlir/lib/Dialect/Linalg/Utils/Utils.cpp:156
+  }
+  auto nestedFn = [&]() {
+    GenerateLoopNest<scf::ParallelOp>::doit(
----------------
nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80188