Skip to content

[LoopInterchange] Redundant isLexicographicallyPositive check #117343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[LoopInterchange] Redundant isLexicographicallyPositive check #117343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions llvm/lib/Transforms/Scalar/LoopInterchange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,12 @@ static bool isLegalToInterChangeLoops(CharMatrix &DepMatrix,
std::vector<char> Cur;
// For each row check if it is valid to interchange.
for (unsigned Row = 0; Row < NumRows; ++Row) {
// Create temporary DepVector check its lexicographical order
// before and after swapping OuterLoop vs InnerLoop
// A dependence vector has to be lexigraphically positive. We could assert
// that here, as a sanity check for the dependency analysis, but it can
// contain "don't know" elements ('*'), which will be rightfully
// interpreted as not lexicographical positive. So, skip that sanity check,
// it suffices just to check the interchanged direction vector.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, there was a reason for isLexicographicallyPositive(Cur) to be called before the swap. I did not recall explicitly but it likely has something to do with 'S' or '*' dependency. We had encountered a number of bugs due to the fact that 'S' and '*' not very easily dealt with in loop interchange.

If you remove the isLexicographicallyPositive() call here, likely bugs would appear. I probably do not have enough time digging into it at the moment, but you may refer to this patch (https://reviews.llvm.org/D137461) and also earlier patches and issues in loop interchange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that pointer.

I agree with the following from that phabricator review, that's what I am saying too:

At the LoopWG call we discussed that we still could call isLexicographicallyPositive before the interchange to check that the dependency is understood before risting breaking things. I don't think it's necessary, but I don't mind.

The impression that I get from that discussion and the mishandling of scalar dependencies, is that we are not really sure why we are doing things.

Anyway, I don't have too strong opinions on this, I still don't think it is necessary, but I will focus on #119345 first.

Copy link
Contributor

@kasuga-fj kasuga-fj Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I found this PR by chance. I think isLexicographicallyPositive check is necessary before the interchange.

if there's a "don't know" element, then the permuted direction vector will be rejected too.

I think the following case is a counterexample to this.

#define N 4
int a[N][N];
void f() {
  for (int i = 0; i < N; i++) {
    for (int j = 0; j < N-1; j++) {
      a[j][N-1-i] += a[j+1][i];
    }
  }
}

Please see also: https://godbolt.org/z/roP9qc7PK

There is a direction vector [* <], which is interpreted as not lexicographically positive before the interchange, but positive after it.

If you want to reduce compilation time, I think one way would be to stop all the subsequent processes and exit when we find a direction vector with * on its first element. However, with improving the legality check, I believe some of these cases should be interchangeable in the future.

Cur = DepMatrix[Row];
if (!isLexicographicallyPositive(Cur))
return false;
std::swap(Cur[InnerLoopId], Cur[OuterLoopId]);
if (!isLexicographicallyPositive(Cur))
return false;
Expand Down
Loading