Skip to content

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" #54176

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.

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

Closed
bmahjour opened this issue Mar 3, 2022 · 3 comments
Assignees

Comments

@bmahjour
Copy link
Collaborator

bmahjour commented Mar 3, 2022

The following example shows a miscompile:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

__attribute__((noinline))
void foo(int n, int m, float aa[1024][128], float bb[1024][128], float cc[1024][128]) {
#ifndef MYINTERCHANGE
  for (int j = 1; j < 128; j++)  {
    for (int i = 1; i < 1024; i++)    {
      aa[1][j-1] += bb[i][j];
      cc[i][j] = aa[1][j];
    }
  }
#else
  for (int i = 1; i < 1024; i++)  {
    for (int j = 1; j < 128; j++)    {
      aa[1][j-1] += bb[i][j];
      cc[i][j] = aa[1][j];
    }
  }

#endif
}

__attribute__((noinline))
void init(int n, int m, float aa[restrict][m]) {
  for (int i = 0; i < n; i++)  {
    for (int j = 0; j < m; j++)    {
      aa[i][j] = i*j;
    }
  }
}

__attribute__((noinline))
void print(int n, int m, float aa[restrict][m]) {
  for (int i = 0; i < n; i++)  {
    for (int j = 0; j < m; j++)    {
      printf("[%d,%d]=%f\n", i, j, aa[i][j]);
    }
  }
}

#define N 1024
#define M 128

int main() {
  float *f1, *f2, *f3;
  f1 = (float *) malloc (N * M * sizeof(float));
  f2 = (float *) malloc (N * M * sizeof(float));
  f3 = (float *) malloc (N * M * sizeof(float));
  memset(f3, 0, N*M*sizeof(float));
  init(N,M, (float (*)[])f1);
  init(N,M, (float (*)[])f2);
  foo(N,M, (float (*)[])f1, (float (*)[])f2, (float (*)[])f3);
  print(N,M,(float (*)[])f3);
}
> clang  bad-interchange.c -O3 -o bad -mllvm -enable-loopinterchange
> clang  bad-interchange.c -O3 -o good
> good > 1.txt && bad > 2.txt
> diff 1.txt 2.txt | head
258,383c258,383
< [2,1]=1.000000
< [2,2]=2.000000
@bmahjour
Copy link
Collaborator Author

bmahjour commented Mar 3, 2022

The problem is that S dependencies should be treated as if having loop carried dependency in all directions and they don't!
There is also a problem in how the legality analysis handles dependency vectors that have a ">" in their left-most non-'=' position. A slightly modified version of the above test case is treated illegal (which is good) but for the wrong reason:

  for (int j = 1; j < 128; j++)  {
    for (int i = 1; i < 1024; i++)    {
      cc[i][j] = aa[1][j];
      aa[1][j-1] += bb[i][j];
    }
  }

it starts with an row of [-1 S] and then permutes it to [S -1]. It treats the S as if it was a = and only concludes the interchange is illegal because now the left-most non-'=' is >'. What should really be the reason to stop interchange is the presence of Sin the left-most position not the>` in the second position.

In such cases it should reverse all directions within that row, since a dependence whose sink happens before its source is meaningless.

@bmahjour
Copy link
Collaborator Author

bmahjour commented Mar 3, 2022

@bmahjour bmahjour self-assigned this Mar 7, 2022
ShivaChen added a commit to ShivaChen/llvm-project that referenced this issue Jan 23, 2024
…e other has loop carried dependence

Consider the following loop from
llvm#54176

  for (int j = 1; j < M; j++)
    for (int i = 1; i < N; i++) {
      aa[1][j-1] += bb[i][j];
      cc[i][j] = aa[1][j];
    }

Loops should not be interchanged in this case as it will change the
cc array results.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Dec 10, 2024
This makes loop-interchange more pessimistic, but also more correct as
we are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
lift this restriction.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 7, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch checks that the dependency matrix has no "S" direction as the
leftmost non-"=" direction in any row. This is a stopgap that prevents
the miscompiles, at the expense of handling less cases, i.e. making
interchange more pessimistic. However, some of the cases that are now
rejected for dependence analysis reasons, were rejected before for other
reasons (e.g. profitability). So at least for the llvm regression tests,
the number of regression are very reasonable.

This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
lift this restriction.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 13, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 16, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 17, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 20, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
sjoerdmeijer added a commit to sjoerdmeijer/llvm-project that referenced this issue Jan 20, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" llvm#54176
[LoopInterchange] Interchange breaks program correctness llvm#46867
[LoopInterchange] Loops should not interchanged due to dependencies llvm#47259
[LoopInterchange] Loops should not interchanged due to control flow llvm#47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
sjoerdmeijer added a commit that referenced this issue Jan 20, 2025
We are not handling 'S' scalar dependencies correctly and have at least
the following miscompiles related to that:

[LoopInterchange] incorrect handling of scalar dependencies and dependence vectors starting with ">" #54176
[LoopInterchange] Interchange breaks program correctness #46867
[LoopInterchange] Loops should not interchanged due to dependencies #47259
[LoopInterchange] Loops should not interchanged due to control flow #47401

This patch does no longer insert the "S" dependency/direction into the
dependency matrix, so a dependency is never "S". We seem to have
forgotten what the exact meaning is of this dependency type, and don't
see why it should be treated differently.

We prefer correctness over incorrect and more aggressive results. I.e.,
this prevents the miscompiles at the expense of handling less cases,
i.e. making interchange more pessimistic. However, some of the cases
that are now rejected for dependence analysis reasons, were rejected
before too but for other reasons (e.g. profitability). So at least for
the llvm regression tests, the number of regression are very reasonable.
This should be a stopgap. We would like to get interchange enabled by
default and thus prefer correctness over unsafe transforms, and later
see if we can get solve the regressions.
@kasuga-fj
Copy link
Contributor

Should have been fixed by #119345

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants