[PATCH] D13487: [Polly] Load/Store scalar accesses before/after the statement itself

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

There are 3 major points to this patch:

1. The code movement between the copyBB methods seem not necessary and should be avoided to simplify the insertion point requirements needed.
2. The dominance check (as well as the isPHI check before that) are complicated and I still do not follow what/how they are different to what was happening before. I think they somehow simplify the generated code but that only hides the underlying issue
3. The general idea of one map and the early reload of scalars seems reasonable and looks good except what is mentioned in point 1) and 2).


================
Comment at: lib/CodeGen/BlockGenerators.cpp:341
@@ -339,1 +340,3 @@
+  // their alloca.
+  generateScalarStores(Stmt, LTS, BBMap);
   return CopyBB;
----------------
If we keep the SetInsertPoint and the generateScalarXXX calls in the other copyBB function we don't need to set the insert point manually outside (noted somewhere below). If there is a reason you moved it here please explain.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1035
@@ -1043,7 +1034,3 @@
     BlockMap[BB] = BBCopy;
-
-    // Get the mapping for this block and initialize it with the mapping
-    // available at its immediate dominator (in the new region).
-    ValueMapT &RegionMap = RegionMaps[BBCopy];
-    RegionMap = RegionMaps[BBCopyIDom];
+    ValueMap[BB] = BBCopy;
 
----------------
BlockMap seems to be obsolet if we store the mapping in the ValueMap too. If so we could remove it now or later.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1038
@@ -1050,5 +1037,3 @@
     // Copy the block with the BlockGenerator.
-    copyBB(Stmt, BB, BBCopy, RegionMap, LTS, IdToAstExp);
-
-    // In order to remap PHI nodes we store also basic block mappings.
-    BlockMap[BB] = BBCopy;
+    Builder.SetInsertPoint(BBCopy->begin());
+    copyBB(Stmt, BB, BBCopy, ValueMap, LTS, IdToAstExp);
----------------
As mentioned above, this complicates the copyBB logic and could be hidden.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1195
@@ -1169,1 +1194,3 @@
+               !DT.dominates(cast<Instruction>(Val)->getParent(), StmtExitBB))
+      continue;
 
----------------
As mentioned in the phone call, we should never need to check dominance here as we didn't do it before. If this patch somehow requires that, it might not be the best way to do it. 

Can you illustrate why this is needed (with an example) and why it is beneficial not to store the scalars as before? We simply generated scalar stores in the blocks they were defined and did not need to reason about insertion points and dominance at all.

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll:20
@@ -19,9 +19,3 @@
 ; CHECK-NEXT: %p_val2 = fadd float 1.000000e+00, 2.000000e+00
-; CHECK-NEXT: store float %p_val0, float* %merge.phiops
-; CHECK-NEXT: store float %p_val1, float* %val1.s2a
-; CHECK-NEXT: store float %p_val2, float* %val2.s2a
-
-; FIXME -> The last two writes are not really needed and can be dropped if the
-;          incoming block of the PHI and the value that is used share the same
-;          non-affine region.
+; CHECK: store float %p_val0, float* %merge.phiops
 
----------------
While the result looks nice I think the way we achieved is not what we want. We should not generate the scalar write accesses in the SCoP if they are not needed but now we apparently filter them in the code generation.


http://reviews.llvm.org/D13487