[PATCH] D36647: [Polly][WIP] Scalar fully indexed expansion

[Andreas Simbuerger via Phabricator via llvm-commits]

simbuerg added a comment.

We talked about it on Friday but I think I can help out more with a small writeup.

First to your SCoP. The accesses to 'add_lcssa_phi' result from LLVM transforming the loop into "loop closed static single assignment" form, i.e.,
providing a single definition for values that live accross the loop boundaries outside of the loop. LLVM does this by inserting redundant PHI nodes for these values.
That's why you see a PHI node with a single operand in the LLVM IR right after the innermost loop.

You got (roughly) this after LCSSA (actually only tmp_lcssa):

  #define Ni 10000
  #define Nj 10000
  void mse(double A[Ni], double B[Nj]) {
    int i,j;
    double tmp = 6;
    for (i = 0; i < Ni; i++) {
      // Either outside, or previous
      tmp_0 = PHI(tmp, tmp_lcssa);
  
      for (int j = 0; j<Nj; j++) {
        // Either outside, or previous
        tmp_1 = PHI(tmp_0, tmp_2);
        tmp_2 = tmp_1 + 2;
      }
  
      // tmp_2's lifetime exceeds the innermost loop
      tmp_lcssa = PHI(tmp_2);
      B[i] = tmp_lcssa;
    }
  }

Polly then models each PHI node as a 'load' of new memory that was written at the end of the incoming block (numbering is differnt, sorry).

  Statements {
  	Stmt_for_body
          ReadAccess :=	[Reduction Type: NONE] [Scalar: 1] // Load current value of 'tmp' (PHI_0)
              { Stmt_for_body[i0] -> MemRef_tmp_04__phi[] };
          MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 1] // Write for PHI_1
              { Stmt_for_body[i0] -> MemRef_tmp_11__phi[] };
          
  	Stmt_for_inc
          MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 1] // Write for PHI_1
              { Stmt_for_inc[i0, i1] -> MemRef_tmp_11__phi[] };
          ReadAccess :=	[Reduction Type: NONE] [Scalar: 1] // Load current value of 'tmp' (PHI_1)
              { Stmt_for_inc[i0, i1] -> MemRef_tmp_11__phi[] }; 
          MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 1] // Write for PHI_2
              { Stmt_for_inc[i0, i1] -> MemRef_add_lcssa__phi[] };
          
  	Stmt_for_end
          MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 1] // Write for PHI_0
              { Stmt_for_end[i0] -> MemRef_tmp_04__phi[] };
          ReadAccess :=	[Reduction Type: NONE] [Scalar: 1] // Load current value of 'tmp_lcssa' (PHI_2)
              { Stmt_for_end[i0] -> MemRef_add_lcssa__phi[] };
          MustWriteAccess :=	[Reduction Type: NONE] [Scalar: 0]
              { Stmt_for_end[i0] -> MemRef_B[i0] };
  }

Now when you switch the roles of Reads and Writes for PHI nodes you should always get a single read and one or more writes (see above). After PHI expansion it should virtually look like this:

  void mse(double A[Ni], double B[Nj]) {
    int i,j;
    double tmp = 6;
    for (i = 0; i < Ni; i++) {
      // Either outside, or previous
      tmp_0 = PHI(tmp, tmp_lcssa); // READ: tmp_0[i] (Problem: Nobody is writing this value
                                                          (Copy in required), you should actually bail out on this example).
      // WRITE: tmp_1[i][0] (Initial value).
  
      for (int j = 0; j<Nj; j++) {
        // Either outside, or previous
        tmp_1 = PHI(tmp_0, tmp_2); // READ: tmp_1[i][j]
        tmp_2 = tmp_1 + 2;
  
        // WRITE: tmp_1[i][j+1] (We need to write the current value into the 'next' iteration because we will read the value from there).
        // WRITE: tmp_lcssa[i] (Got this one from LCSSA)
      }
  
      tmp_lcssa = PHI(tmp_2); // READ: tmp_lcssa[i]
      B[i] = tmp_lcssa;
  
      // WRITE: tmp_0[i+1] (We need to write the current value into the 'next' iteration because we will read the value from there).
    }
  }

I can't see yet why your code specifically does the wrong thing, but I hope this helps anyway. @everyone correct me if i missed something, it was late :-)



================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:398
 
-    assert(AllWrites.size() == 1);
+    // If MemoryKind::Value of MemoryKind::Array
+    if (SAI->isValueKind() || SAI->isArrayKind()) {
----------------
or not of.

Besides, no need to verbally describe the if statement. Just remove the comment.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:406
+      for (MemoryAccess *MA : AllReads)
+        expandAccordingToDependences(S, MA, Dependences, ExpandedArray, true);
+    }
----------------
Why 'expandAccordingToDependences'?

Style: I would handle that loop inside expandRead(s) and pass the Reads as parameter (You have to handle them all anyway inside  the function, might as well iterate in there).

I think what would make everything clearer (to me at least) would be a naming as follows:

expandAccess (Read/Write): Your expandAccordingToDomain.
redirect/mapAccess (Read/Write): Your expandAccordingToDependences.

The first does the array allocation, the second redirects/maps the access(es) to the new array.


https://reviews.llvm.org/D36647