[PATCH] Scalar/PHI code genration

[Johannes Doerfert]

Quick comment about lnt:

  We mostly benefit from this (as 5 runs on my laptop show) and do not regress to badly anywhere.

My last concern:

  I haven't tested this with parallel code generation yet. We need to do that before enabling it (but not before commiting it though).


================
Comment at: include/polly/CodeGen/BlockGenerators.h:121
@@ +120,3 @@
+  ScalarAllocaMapTy PHIOpMap;
+  ScalarAllocaMapTy ScalarMap;
+  ///}
----------------
grosser wrote:
> 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.
I added a comment for both maps to describe what they are used for.

================
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.
+  ///
----------------
grosser wrote:
> 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).
I removed this. I use generateScalarStorePHI now from generateScalarStore and the ge cnerateScalarLoad code is easy for both cases.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:321
@@ +320,3 @@
+      InstMapped = InstCopy;
+    }
+  }
----------------
grosser wrote:
> jdoerfert wrote:
> > grosser wrote:
> > > 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;
> > > ```
> > During the development this looked different and when I changed it I assumed that if there is a inst copy there is a mapping, however for synthezisable instructions there is none (currently), the same holds for phi nodes (but then there is no inst copy either).
> I understand partially. ;-)
> Are you saying this is needed for PHI nodes and synthezisable instructions, but we just don't have a test case? Or this was needed at some point, but is not needed any more?
> 
I put synthezised instructions in the BBMap and removed the conditional you asked about.

http://reviews.llvm.org/D7513

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