[PATCH] Scalar/PHI code genration

[Johannes Doerfert]

answers


================
Comment at: include/polly/CodeGen/BlockGenerators.h:121
@@ +120,3 @@
+  ScalarAllocaMapTy PHIOpMap;
+  ScalarAllocaMapTy ScalarMap;
+  ///}
----------------
grosser wrote:
> 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.
For almost all corner cases and most of the decisions I need to add unit tests (I know).

Nevertheless, I am quite sure you need two allocations for a PHI node, the general test case structure looks like:

```
x1 = ...
for (i=0...N) {
  x2 = phi(x1, add)
  add = x2 + A[i];
}

print(x1)
print(x2)
print(add)

```

================
Comment at: lib/CodeGen/BlockGenerators.cpp:463
@@ +462,3 @@
+    int ScalarBasePHIIdx =
+        ScalarBasePHI ? ScalarBasePHI->getBasicBlockIndex(BB) : -1;
+
----------------
grosser wrote:
> This could be folded into the if(ScalarBasePHI) condition, no?
It is used twice so I moved it out of the two if cases.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:476
@@ +475,3 @@
+    AllocaInst *ScalarAddr =
+        getOrCreateAlloca(ScalarBase, ScalarBasePHI ? PHIOpMap : ScalarMap);
+
----------------
grosser wrote:
> 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.
We could extract the phi path probably, but I still need we need 2 maps.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:482
@@ +481,3 @@
+    // value, for all others we have to reload them first.
+    Value *ScalarValue = ScalarInst;
+
----------------
grosser wrote:
> 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.
> 
ok

================
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.
----------------
grosser wrote:
> PHI? You sometimes write it uppercase, otherwise lowercase.
True. Which do you prefer?

================
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);
----------------
grosser wrote:
> Why would we reload a value to directly store it back? Could we instead just neither load nor store the value?
Unit test idea:

```
bb1:
  %x = 
  br %cond, label %then, label %else

then:
  br label %merge

else:
  %y = 

merge:
  %p = phi[(%x, %then), (%y, %else)]
```

Now we have a write access in the %then block which needs to reload %x and store it in the operand location of %p.

http://reviews.llvm.org/D7513

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