[PATCH] [Polly] Model PHI nodes without demoting them

Tobias Grosser tobias at grosser.es
Wed Feb 4 23:20:18 PST 2015


Hi Johannes,

the patch looks good in general. As you mentioned earlier, the test coverage is still a little low. I would be interested in a loop with a non-IV PHI node, as well as two tests where the values in the PHis are instructions, once executed in the incoming basic blocks and once in a block that is post-dominated by the relevant incoming block. What do you think?

Cheers,
Tobias


================
Comment at: lib/Analysis/ScopDetection.cpp:155
@@ +154,3 @@
+    cl::desc(
+        "Allow PHI nodes in the input [Unsafe with any code-generation!]."),
+    cl::location(PollyModelPHINodes), cl::Hidden, cl::ZeroOrMore,
----------------
without

================
Comment at: lib/Analysis/TempScopInfo.cpp:102
@@ +101,3 @@
+void TempScopInfo::buildPHIAccesses(PHINode *PHI, Region &R,
+                                    AccFuncSetType &Functions) {
+  if (canSynthesize(PHI, LI, SE, &R))
----------------
Maybe a comment on how we handle PHI accesses, where loads/stores are created.

================
Comment at: lib/Analysis/TempScopInfo.cpp:111
@@ +110,3 @@
+    if (!R.contains(OpBB))
+      continue;
+
----------------
Can this ever happen? If it can, maybe this is worth a test case, otherwise maybe make this an assert?

================
Comment at: lib/Analysis/TempScopInfo.cpp:229
@@ -200,1 +228,3 @@
+    }
+
     if (!isa<StoreInst>(Inst) && buildScalarDependences(Inst, &R)) {
----------------
No need for braces?

http://reviews.llvm.org/D7415

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list