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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp:387
+  GenericLoopNestRangeBuilder<LoopTy>(ivs, linalgRanges,
+                                      op.iterator_types().getValue())([&] {
     auto &b = ScopedContext::getBuilderRef();
----------------
nicolasvasilache wrote:
> 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?
Thanks for pointing this out. Added a test to verify the behavior as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80188