[PATCH] Scalar/PHI code genration

[Tobias Grosser]

Hi Johannes,

the general structure of the patch looks very good. I started directly to go through it in detail to try to understand what finer details and the corner cases/difficulties you have addressed.

I did not yet manage to finish the patch, but wanted to already send you my first comments.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:121
@@ +120,3 @@
+  ScalarAllocaMapTy PHIOpMap;
+  ScalarAllocaMapTy ScalarMap;
+  ///}
----------------
Is there a reason you use two different maps. As an instruction is either a
scalar or a phi, you could just store them in a single map, no? This might avoid the need to later choose between the two.

While reading further through the source code, it seems a PHI instruction may have two allocations. One for the incoming values and one for the outcoming values. Maybe it would be worth a comment and an example here that explains which values belong where and why PHI nodes can require two different allocs.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:461
@@ +460,3 @@
+    Instruction *ScalarInst = MA->getAccessInstruction();
+    PHINode *ScalarBasePHI = dyn_cast<PHINode>(ScalarBase);
+    int ScalarBasePHIIdx =
----------------
The non-PHI case is pretty straightforward, but it becomes unclear due to the mix of PHI and non-PHI code. My comments below try to point out how those code paths could be made more separate.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:463
@@ +462,3 @@
+    int ScalarBasePHIIdx =
+        ScalarBasePHI ? ScalarBasePHI->getBasicBlockIndex(BB) : -1;
+
----------------
This could be folded into the if(ScalarBasePHI) condition, no?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:465
@@ +464,3 @@
+
+    // Skip the artifical self write access of phi nodes as they are handled
+    // when the read access is treated. Also see generateScalarPHI(...).
----------------
artificial.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:476
@@ +475,3 @@
+    AllocaInst *ScalarAddr =
+        getOrCreateAlloca(ScalarBase, ScalarBasePHI ? PHIOpMap : ScalarMap);
+
----------------
A single map might avoid this ternary condition on PHIOpMap, no?

Also, moving this below the if(ScalarBasePHI) block, would allow to move the continue for artificial self write accesses into the ScalarBasePHI block as well. Like this there is just a single block that contains all the PHI special cases.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:482
@@ +481,3 @@
+    // value, for all others we have to reload them first.
+    Value *ScalarValue = ScalarInst;
+
----------------
You could possibly put this into an

if (!ScalarBasePHI) {
 ScalarValue = ScalarInst;
} else {

}

This would make clear that these are two exclusive code paths. One for the common case, one for the PHI.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:484
@@ +483,3 @@
+
+    // In a first step recognize phi operand stores and get the right value
+    // from the phi operand list.
----------------
PHI? You sometimes write it uppercase, otherwise lowercase.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:509
@@ +508,3 @@
+    //       - If so, store the ScalarValue in the ScalarAddr.
+    //       - Otherwise, check the the next point.
+    //   - The ScalarValue instruction was mapped in the current basic block:
----------------
typo.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:512
@@ +511,3 @@
+    //       - If so, store the new version of the ScalarValue.
+    //       - Otherwise, reload the ScalarValue and store the reloaded value.
+    Instruction *ScalarValueInst = dyn_cast<Instruction>(ScalarValue);
----------------
Why would we reload a value to directly store it back? Could we instead just neither load nor store the value?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:543
@@ +542,3 @@
+
+  // For each phi predecessor outside the region store the incoming operand
+  // value prior to entering the optimized region.
----------------
PHI?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:562
@@ +561,3 @@
+void BlockGenerator::createScalarFinalization(Region &R) {
+  // The exit block of the __unoptimzed__ region.
+  BasicBlock *ExitBB = R.getExitingBlock();
----------------
typo

================
Comment at: lib/CodeGen/BlockGenerators.cpp:594
@@ +593,3 @@
+
+    // The information of scalar evlution about the escaping instruction needs
+    // to be revoked so the new merged instruction will be used.
----------------
typo

http://reviews.llvm.org/D7513

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