Page MenuHomePhabricator

[Polly] Check the properties of accesses to operands of a matrix-matrix multiplication
ClosedPublic

Authored by gareevroman on Sep 25 2021, 10:38 PM.

Details

Summary
The following code modifies elements of the array D.

    for (i = 0; i < _PB_NI; i++)
      for (j = 0; j < _PB_NJ; j++)
      {
        for (k = 0; k < _PB_NK; ++k)
        {
          double Mul = A[i][k] * B[k][j];
          D[i][j][k] += Mul;
          C[i][j] += Mul;
        }
      }

Nevertheless, the code is recognised as a matrix-matrix multiplication, since the second and third dimensions of D are accessed with non-zero strides.

This fixes the typo, which was made during the translation to C++ bindings (https://reviews.llvm.org/D35845).

Diff Detail

Event Timeline

gareevroman created this revision.Sep 25 2021, 10:38 PM
gareevroman requested review of this revision.Sep 25 2021, 10:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2021, 10:38 PM
Meinersbur accepted this revision.Sep 26 2021, 5:26 PM

Thanks for the patch. I have one concern with the the regression test: A single CHECK-NOT test is not a robust test to check anything, the test would succeed even if nothing is printed at all.

I am still accepting the patch in the interest of getting this fixed. However, I may replace the test by something else, e.g. by checking that the output with and without the flag is the same.

This revision is now accepted and ready to land.Sep 26 2021, 5:26 PM

The test need an assertions-enabled build (REQUIRES: asserts) to print anything. That is, it will always pass on non-assert builds.

The test need an assertions-enabled build (REQUIRES: asserts) to print anything. That is, it will always pass on non-assert builds.

Thanks for the review! I have updated the test case to take it into the account.

I am still accepting the patch in the interest of getting this fixed. However, I may replace the test by something else, e.g. by checking that the output with and without the flag is the same.

Could the ast be checked for this purpose?

Could the ast be checked for this purpose?

Yes, that's a possibility.