[RFC] preserving loop metadata

[Pekka Jääskeläinen]

Hi Paul,

On 02/20/2013 10:20 PM, Redmond, Paul wrote:
> PreserveLoopMetadata is intended to be used by LoopPasses to preserve the
> loop metadata after transformations. I have modified LoopRotation as an
> example (it is a pass which will cause the metadata to get lost) The classes
> is also intended to make the changes to existing passes as unobtrusive as
> possible.

Sounds good. The idea seems similar to what I had in mind: a separate
pass/utility code that records the loop MD before an "intrusive optimization"
which can be then "recovered" afterwards.

The main concern I had with that approach is what to do with non-LoopPasses.
If the construct at hand is not a Loop, how nicely to record the loop MD and
fix it for the loop body we modified (e.g. inlining a function into a
parallel loop body)?



Some comments to the patch itself:

+static StringRef getParallelLoopName() {
+  return "llvm.loop.parallel";
+}
+
+static StringRef getParallelAccessName() {
+  return "llvm.mem.parallel_loop_access";
+}

Is this a common idiom in LLVM; to have static functions that
return constant strings? Does it break something (e.g.
global object construction order issues) to have them actually
constant strings in the LoopMetadata class or similar?
In any case the naming could be improved, e.g.,
getParallelLoopName() to getParallelLoopMDName().

+static MDNode *getParallelLoop(const Loop *L) {
-->
Perhaps rename to getParallelLoopID() or similar. As this
returns the MDNode that uniquely marks a loop, not the loop itself.

Does the multiple-predecessor search for loop ID work correctly when you
have multiple levels of parallel loops (the basic case in OpenCL
kernels)? In that case some preheader might have multiple loop
identifiers in a list, one for each level, right? Please add a unit
test for the multilevel case when you prepare the final patch to ensure
there are no problems with this.

+  LoopMD = getParallelLoop(TheLoop);
+  if (LoopMD && !checkParallelLoopForAccess(TheLoop, LoopMD))
+    LoopMD = 0;

As you seem to plan using LoopMetadata for tracking a wider set of loop
metadata than the parallel loop MD, maybe LoopMD should be named
ParallelLoopMD.


+PreserveLoopMetadata::PreserveLoopMetadata(Loop *L)
+: LoopMD(L), PreserveParallel(true) {
+  assert(L && "Loop is null");
+}

Addressing the above mentioned concern, maybe this should also be
able to input a Function. In case of inlining, for example, the
inlining should not break any parallel loops so the MD in
all the loops inside the Function at hand should be preserved?
Maybe something to consider if/when someone implements preserving
the info in a FunctionPass.


The RAII approach saves some repetitive lines of code if there are
multiple return points/exceptions in the function at hand, but my
humble opinion on this is that it's often better to be explict than implicit
and the RAII should be used for freeing of resources not for progrma
logic.

E.g., if a programmer doesn't realize that the object does MD updating
in the destructor and adds a return after the object constructor call,
it might lead to hard to track errors if he doesn't want to touch the MD
for some reason.

All in all, I like the approach but it might need extending/redesign
to support preserving the (parallel) Loop MD in Functions for the
FunctionPasses.

-- 
--Pekka