[PATCH] D16522: BlockGenerators: Replace getNewScalarValue with getNewValue

[Michael Kruse via llvm-commits]

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

Bu coincidence, I was working on the very same patch. Since rebasing http://reviews.llvm.org/D15687, a new bug appear for which to solve this is needed. I was still in the process of testing whether there are no other problems. My version of this patch is here <https://github.com/Meinersbur/polly/commit/a114637eb21f9d7c33b62eafb61deeaa6266ef28>


================
Comment at: lib/CodeGen/BlockGenerators.cpp:412
@@ +411,3 @@
+    if (MA->isScalarKind() && GlobalMap.count(MA->getBaseAddr()))
+      continue;
+
----------------
This is fixing a symptom only. The MemoryAccess shouldn't exist in the first place if we are going to ignore it.

In fact, invariant loads do not need any MemoryAccesses inserted at all. One can just use the loaded llvm::Value as it is inserted before the generated code. In case the load is conditional, inserting a phi with an undef on one side is simple.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1222
@@ -1262,2 +1221,3 @@
 
-    Val = getNewScalarValue(Val, R, Stmt, LTS, *LocalBBMap);
+    Loop *L = getLoopForInst(ScalarInst);
+    Val = getNewValue(Stmt, Val, *LocalBBMap, LTS, L);
----------------
This is wrong; we don't need the loop where the value is defined but where it is used. The difference comes in to play when the value is defined in the loop, but we write its scalar value after the loop, thus use the value after the last loop iteration.

================
Comment at: test/Isl/CodeGen/invariant_load_scalar_escape_alloca_sharing.ll:14
@@ -13,3 +13,3 @@
 ; CHECK:       %ncol.load = load i32, i32* @ncol
 ; CHECK-NEXT:  store i32 %ncol.load, i32* %tmp0.preload.s2a
 ;
----------------
This store became useless as well, didn't it?


http://reviews.llvm.org/D16522