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

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In http://reviews.llvm.org/D13487#264224, @jdoerfert wrote:

> 1. The code movement between the copyBB methods seem not necessary and should be avoided to simplify the insertion point requirements needed.


Undone in update

> 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


The dominance check will go away in a separate patch that depends on this one.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:341
@@ -339,1 +340,3 @@
+  // their alloca.
+  generateScalarStores(Stmt, LTS, BBMap);
   return CopyBB;
----------------
jdoerfert wrote:
> 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.
SetInsertPoint needs to be set before generateScalarLoads. RegionGenerator does not call generateScalarLoads before each copyBB. If moving Builder.SetInsertPoint(CopyBB->begin()) into copyBB, this overload would still need to call it before generateScalarStores.

================
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;
 
----------------
jdoerfert wrote:
> BlockMap seems to be obsolet if we store the mapping in the ValueMap too. If so we could remove it now or later.
I aggree, should be a separate change.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1195
@@ -1169,1 +1194,3 @@
+               !DT.dominates(cast<Instruction>(Val)->getParent(), StmtExitBB))
+      continue;
 
----------------
jdoerfert wrote:
> 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.
> 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.

As mentioned in the phone call and a message to the mailing list, the current code does generate such MemoryAccesses. Some later planned change will hopefully make this check go away.


> 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.

Explained in summary message. It is not beneficial by itself, but required for later changes.

================
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
 
----------------
jdoerfert wrote:
> 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.
This is a side-effect because val0 and val1 are dominating the non-affine region's exit, i.e. it is not possible to have such a store in the exit block. A planned change will clean up the logic that generates MemoryAccesses.

I didn't not understand why check for instructions that shouldn't even be there, so I just removed the check.


http://reviews.llvm.org/D13487