[Mlir-commits] [mlir] [mlir][vector] Add multi_reduction_rank_reducing_unrolling (PR #182301)

[Erick Ochoa Lopez]

amd-eochoalo wrote:

> I am a bit confused and that might be because you have added quite a few commits since my last review. ATM this is only adding new code, so not really forcing anyone to anything? I guess that you are referring to the previous state? Could you remind me what that was?

Sure, no problem. Thanks for being patient.

I decided to just add new code because I want to provide more time for downstream projects to migrate. This is just an alternative unrolling strategy that performs rank reduction and can be done instead of flattening first. I'd like to add next the equivalent for innerreduction and then for the vector.contraction operation. That way, one could have rank-reduction unrolling being performed in a single pass.

> In general, we don't really make promises about our C++ APIs and these are subject to change, which is usually painful for downstream users. While not ideal, this how much of LLVM operates. I'm definitely in favour of reducing the pain for our downstream users, but sometimes a helpful comment can be enough (e.g. "In order to preserve the original behaviour, do A -> B -> C").

Thanks! I'll take this into consideration. I think adding more functionality is the right call here though. To preserve the original behaviour users would need to make sure that reorder_and_expand and flattening are run before unrolling. The reason why I prefer to just add this extra case is because users may be applying patterns other than flattening and reorder_and_expand at the same time. It is a bit awkward to separate only this one from all the others. So one would need to track the dependencies between the patterns. E.g., Patterns that produces vector.multi_reduction must also run before / at the same time as reorder_and_expand.

This can be visualized as a three stage process: everything before unrolling, unrolling, everything after unrolling. 

> This makes me realise - why do we need bott TwoDimMultiReductionToElementWise and MultiReductionToArithOps? We should strive to make patterns orthogonal in their functionality.

I realized after making that comment that they are not equivalent. They are only equivalent when rank == 2. Which if the patterns are applied in one single applyPatternsGreedily, then two patterns may match the same operation. When applying multiple patterns at the same time, a single op may match multiple patterns. 



https://github.com/llvm/llvm-project/pull/182301