[PATCH] D76132: [LoopUnrollAndJam] Changed safety checks to consider more than 2-levels loop nest.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In D76132#1943872 <https://reviews.llvm.org/D76132#1943872>, @Whitney wrote:

> Thanks @Meinersbur ! I mostly used your code directly, except
>
>   for (unsigned d = 1; d < UnrollLevel; ++d) {
>         // Check if dependence is carried by an outer loop.
>         // That is, changing
>         //   (0,>,>,*,*)
>         // to
>         //   (0,>,>=,*,*)
>         // will still not violate the dependency.
>         if (D->getDirection(d) == Dependence::DVEntry::GT)
>           return true;
>       }
>
>
> which I think should be safe as long as the one dependence is not EQ then should be safe.
>
>   for i
>     for j        <= unroll loop
>       for k
>          A[i][j][k]
>          A[i-1][j+1][k]
>
>
> Loop-j should be safe to unroll and jam. Am I right?


Yes, that's what the cod above would be testing.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:715-718
+    if (any_of(
+            llvm::seq<unsigned>(1, UnrollLevel), [&D](unsigned CurLoopDepth) {
+              return !(D->getDirection(CurLoopDepth) & Dependence::DVEntry::EQ);
+            }))
----------------
[serious] This does not correspond to the `getDirection() == GT` from my version. In particular, this version lets' the loop pass if the outer loops have `NE` or `LT` dependenies. These do not ensure lexicographic greater-than and therefore cause a misoptimization.

[remark] While some style guides prefer this functional style, I prefer the version with explicit loops and the comment that explains what the code is doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132