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

James Molloy james.molloy at arm.com
Tue Jan 27 09:56:05 PST 2015

Hi Hal,


Comments inline.



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.



More information about the llvm-commits mailing list