[LLVMdev] [PATCH] Split LoopUnroll pass into mechanism and policy

[Matthijs Kooijman]

Hi Dan,


> I'm not sure that's very pretty either :-}. My sense is that it's
> better to just duplicate those few lines. 
Then I'll just leave it at that.

> An assert at the end that checks that if the loop
> wasn't completely unrolled that it's still isLCSSAForm would be a
> good addition in any case.
Good idea.

> I don't think it's desirable to require a LoopPassManager object,
> but I think it would be fine to have an optional LoopPassManager
> parameter which can be null for this purpose. If a LoopPassManager
> object is provided and the loop is completely unrolled, the
> loop is removed from the LoopPassManager.
Sounds fine.

> > Lastly, the (original comments) say "This pass will multi-block  loops
> > only if they contain no non-unrolled subloops". i Anyone know what this
> > means?
> I don't.
Then I'll just remove the comment, it's only confusing :-)

> This should say (>= 2^32).
Obviously.

> > +        // Guard against huge trip counts. This also guards against  
> > assertions in
> > +        // APInt from the use of getZExtValue, below.
> > 
> This comment is a little stale now. Just say it guards against huge
> trip counts.
I didn't fully understand that comment either, though it seemed to make some
sense :-) I'll remove it.

> >  /// This means that the actual trip count is always a multiple of the  
> >  /// returned value (don't forget it could very well be zero as well!).
> I know you corrected this elsewhere, but this comment still says the
> multiple could be zero.
Actually, I meant that the trip count could very well be zero, not the
multiple. I'll clarify the comment..

> 
> +  /// Returns 1 if the trip count is unknown or not guaranteed to be  
> the
> +  /// multiple of a constant (which is also the case if the trip  
> count is simply
> +  /// constant, use getSmallConstantTripCount for that case), Will  
> also return 1
> +  /// if the trip count is very large (> 2^32).
> 
> I know you're just refactoring existing code, but as a standalone  utility
> getSmallConstantTripMultiple should really handle the case where the  trip
> count is constant.
Handle it how? By returning that constant? I had thought about this and
decided for returning 0 instead, but now it seems returning the trip count
instead of 1 is a better idea (since that's the largest constant that the trip
count will always be a multiple of). I'll add that.

> Eventually, it should be taught a few more operators than just Mul too, such
> as Shl and And, but you don't need to worry about that now.
Yeah, that would be useful.

> And again >= instead of >.
Willfix.

> > --- include/llvm/Transforms/Utils/UnrollLoop.h  (revision 0)
> > +++ include/llvm/Transforms/Utils/UnrollLoop.h  (revision 0)
> 
> This file misses the comments that all LLVM headers have.
Hmm, those slipped through :-) I'll add them.

> +bool unrollLoop(Loop *L, unsigned Count, LoopInfo* LI);
> 
> Please capitalize this to UnrollLoop. LLVM is a little inconsistent
> about capitalization in some ways, but standalone transforming
> utility functions are pretty consistently capitalized.
Okay.

Thanks for reviewing the patch. I'll incorporate the proposed changes when I'm
back in office next tuesday.

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080509/c55ece50/attachment.sig>