[PATCH] D136277: [LoopInterchange] Simplify DepMatrix to a dependency vector.

Michael Kruse via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 1 16:14:40 PDT 2022


Meinersbur added a comment.

In D136277#3899782 <https://reviews.llvm.org/D136277#3899782>, @bmahjour wrote:

> Is the size of dependency matrix proving to be problematic for certain workloads?

I am not aware of concrete problems, I mostly felt that it would not be necessary to always go through the entire list of pairwise dependencies and instead could be summarized since which instructions induce the dependencies is irrelevant.

However, due to experimenting with it I now understand that there indeed is a difference than what is done now, although I am not sure whether that situation is sufficiently common.

In D136277#3899782 <https://reviews.llvm.org/D136277#3899782>, @bmahjour wrote:

> On a different note, I think we can make the code more elegant and robust by encapsulating all the dependency matrix analysis inside a class. For example:
>
>   class DepMatrix {
>   ....

Could we call it something else than a matrix? In a matrix, I'd expect the indices have some relevance, here the outer dimension is an unordered collection  (`DenseSet<SmallString<4>>`? That would also de-duplicate identical dependence vectors and thus less work later. Or just a flat array `SmallVector<char>` managed by `DepMatrix`). Also, `std::vector` of `std::vector`s is not the most elegant, esp when resizing.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:84
 // Maximum number of dependencies that can be handled in the dependency matrix.
-static const unsigned MaxMemInstrCount = 100;
+static const unsigned MaxMemInstrCount = 10;
 
----------------
bmahjour wrote:
> why does this need to be reduced so much?
Without this patch, `MaxMemInstrCount` does not compare to the number of instructions, but the number of instruction pairs (that have a dependency). That is, the constant name and the comment above it are inconsistent.

Since in worst case, there can be a quadratic number of pairs, I just took the square of it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:179
+    DepFlags Lvl = DepMatrix[i];
+    if (Lvl & LT || Lvl & GT)
       return false;
----------------
congzhe wrote:
> Please correct me if I'm wrong - IIUC `Scalar` is considered a type of dependence that prevents interchange?
The original code explicitly ignores scalar types (`DepMatrix[Row][i] != 'S'`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136277



More information about the llvm-commits mailing list