[PATCH] D15681: [Polly] Unique phi write accesses

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Michael,

this patch looks good to me. I only had two minor comments specific to this patch and one comment regarding all test case changes in this as well as the read/write uniqueing patches.

LGTM, assuming comments are addressed.

Best
Tobias


================
Comment at: include/polly/ScopInfo.h:612
@@ +611,3 @@
+  /// @param IncomingBlock The PHI's incoming block.
+  /// @param IncomingValue The value when reacing the PHI from the @p
+  ///                      IncomingBlock.
----------------
reaching

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1248
@@ +1247,3 @@
+      } else {
+        // Create a PHI of all possible outgoing values of this subregion.
+
----------------
It might be worth to move the "Create a PHI of all possible outgoing values of this subregion" functionality into its own function.

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll:13
@@ -12,4 +12,3 @@
 ; CHECK: polly.stmt.bb9:                                   ; preds = %polly.stmt.bb3
-; CHECK:   store double 1.000000e+00, double* %tmp12.phiops
 ; CHECK:   br label %polly.stmt.bb11.exit
 
----------------
It would be nice to convert this test case in an earlier commit to the 'CHECK-NEXT' style,
such that by removing the CHECK lines we actually know that the store won't be created any more. Some of the other test cases you modify already use the CHECK-NEXT style. For those just dropping the lines already ensures the lines never occur. If the CHECK-NEXT style would add a lot of code, you might want to add a CHECK-NOT line that verifies the store is not added any more. (Thought I prefer CHECK-NEXT style).

This comment holds for all test cases (where it applies) and possibly also for some of the earlier unique load/store patches.

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll:20
@@ -20,1 +19,3 @@
+; CHECK:   %polly.tmp12 = phi double [ 1.000000e+00, %polly.stmt.bb9 ], [ 2.000000e+00, %polly.stmt.bb10 ]
+; CHECK:   store double %polly.tmp12, double* %tmp12.phiops
 
----------------
CHECK-NEXT?

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll:34
@@ +33,3 @@
+; CHECK-LABEL: polly.stmt.backedge.exit:
+; CHECK:         %polly.merge = phi float [ %p_val0, %polly.stmt.loop ], [ %p_val1, %polly.stmt.branch1 ], [ %p_val2, %polly.stmt.branch2 ]
+
----------------
CHECK-NEXT?

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-4.ll:38
@@ +37,3 @@
+; CHECK-LABEL: polly.stmt.backedge.exit:
+; CHECK:         %polly.merge = phi float [ %p_val0, %polly.stmt.loop ], [ %p_val1, %polly.stmt.branch1 ], [ %p_val2, %polly.stmt.branch2 ]
+
----------------
CHECK-NEXT?

================
Comment at: test/ScopInfo/NonAffine/non_affine_loop_used_later.ll:42
@@ -41,3 +41,3 @@
 ; CHECK:                [N] -> { Stmt_bb4__TO__bb18[i0] -> MemRef_smax[] };
-; CHECK:            MayWriteAccess := [Reduction Type: NONE] [Scalar: 1]
+; CHECK:            MustWriteAccess := [Reduction Type: NONE] [Scalar: 1]
 ; CHECK:                [N] -> { Stmt_bb4__TO__bb18[i0] -> MemRef_j_2__phi[] };
----------------
These test cases would also benefit from an (ahead-of-time) CHECK-NEXT conversion.


http://reviews.llvm.org/D15681