[PATCH] [LoopReroll] Refactor most of reroll() into a helper class

[James Molloy]

Hi Hal,

Thanks!

Comments inline.

James


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:143
@@ +142,3 @@
+    // DAGRootTracker can access analyses such as SCEV.
+    friend class DAGRootTracker;
+
----------------
hfinkel wrote:
> I'd prefer that we not do this just for access to the analysis variables. Just pass what is needed to the child class's constructor.
Sure, I'll make that change.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:698
@@ +697,3 @@
+        if (Roots[Idx-1])
+          // No duplicates allowed.
+          return false;
----------------
hfinkel wrote:
> IIRC, the current version deals with duplicated. I'm not sure this is necessary (one could certainly argue that CSE should take care of anything interesting). Please explain why, however, you're ignoring duplicates in this comment.
That is true. The main reason is simplification of the code: I'm going to be expanding the model to deal with the case with multiple root sets: consider:

    a[i*2+0] = ...; // one root set
    a[i*2+1] = ...;
    a[i*2+4] = ...; // next root set
    a[i*2+5] = ...;
    a[(i+45)*2+0] = ...; // and another root set
    a[(i+45)*2+1] = ...;

And if one root set can have duplicate indices, it adds another level of indirection I need to look through.

It's doable, but as you say it should be unlikely to be needed given a trivial CSE.

http://reviews.llvm.org/D7197

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