[PATCH] Scalar/PHI code genration

[Tobias Grosser]

Second set of comments. Again, mostly comments to help me understand better.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:121
@@ +120,3 @@
+  ScalarAllocaMapTy PHIOpMap;
+  ScalarAllocaMapTy ScalarMap;
+  ///}
----------------
jdoerfert wrote:
> 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)
> 
> ```
Right. I agree that two different allocations might be needed. If you could comment in the code (possibly with example), why PHI nodes need two allocations that would be great. Also, I wonder if you can somehow explain which of the two allocations is "normal" and ends up in the ScalarMap and which is special and ends up in the PHIOpMap, that would help my understanding.

================
Comment at: include/polly/CodeGen/BlockGenerators.h:153
@@ +152,3 @@
+  /// designated to this phi. It will also map the phi to its current value in
+  /// this block.
+  ///
----------------
You explain that this function generates both the loads and the stores of a PHI node, but it is not 100% clear to me why these to need to be generated together, instead of just using the same code-generating for generating their loads/stores as is used for normal scalar loads & stores. Is this for correctness reasons or something else? (In fact, for some PHIs you seem to do so, only self reverencing PHIs seem to be special).

================
Comment at: lib/CodeGen/BlockGenerators.cpp:321
@@ +320,3 @@
+      InstMapped = InstCopy;
+    }
+  }
----------------
I do not understand why this condition (and the forwarding of the instructions in copyInstruction) is necessary. If I remove the full if (InstCopy) block, all test cases still pass for me. Is the BBMap not already updated when the statements are copied?

For example:

```
 // Compute NewStore before its insertion in BBMap to make the insertion
 // deterministic.
​ BBMap[Store] = NewStore;
​ return NewStore;
```

================
Comment at: lib/CodeGen/BlockGenerators.cpp:463
@@ +462,3 @@
+    int ScalarBasePHIIdx =
+        ScalarBasePHI ? ScalarBasePHI->getBasicBlockIndex(BB) : -1;
+
----------------
jdoerfert wrote:
> 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.
I missed the second use.

It is in the special case where you check if a PHI is self-referencing. (I commented earlier that I do not yet understand why stores caused by self-referencing PHIs can not be handled in generateScalarStores. If they could, the second use would not be needed any more).

================
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.
----------------
jdoerfert wrote:
> grosser wrote:
> > PHI? You sometimes write it uppercase, otherwise lowercase.
> True. Which do you prefer?
I do not have a preference. Maybe just check:

grep -R " PHI " lib/ | wc
grep -R " phi " lib/ | wc

and use the one that is more common in Polly.



================
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);
----------------
jdoerfert wrote:
> 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.
Ah, I see. This is also PHI node specific. This indeed makes a lot of sense. mentioning this in the comment and/or folding this into the PHI block might make this more obvious

http://reviews.llvm.org/D7513

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