[llvm-commits] Review request: Hexagon hardware loops

[Hal Finkel]

----- Original Message -----
> From: "Krzysztof Parzyszek" <kparzysz at codeaurora.org>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Thursday, January 24, 2013 10:30:40 AM
> Subject: Re: Review request: Hexagon hardware loops
> 
> On 1/24/2013 10:01 AM, Hal Finkel wrote:
> >
> > +    /// Maximum distance between the loop instr and the basic
> > block.
> > +    /// Just an estimate.
> > +    static const unsigned MAX_LOOP_DISTANCE = 200;
> >
> > Why are you using an estimate? If you know the instruction sizes,
> > don't you know the exact distance which can be compared against
> > the exact limit?
> 
> The code can still change from here on, plus we are changing it here
> by
> adding instructions.  We have no way of making a precise measurement
> at
> this stage.  Doing it later would pose additional difficulties
> (related
> to packetization), so we use an estimate here.

Okay; that makes sense.

> 
> 
> > +    /// The desired flow is: phi ---> bump -+->
> > comparison-in-latch.
> > +    ///                                     +-> back to phi
> > +    /// Due to some prior code transformations, the actual flow
> > may look
> > +    /// like this:
> > +    ///   phi ---> bump ---> back to phi
> > +    ///        +-> comparison-in-latch (against upper_bound-bump)
> >
> > Why do some of the arrows have '+' characters in them?
> 
> ASCII art. :)
> They were meant to represent a join/split point in the graph, but to
> avoid the spread of the comment, the line with the vertical pipe was
> removed (one + sign is missing in the comment):
> 
>    phi ---+--> bump ------> back to phi
>           |
>           +--> comparison-in-latch
> 
> This was supposed to illustrate how the value from the phi follows
> two
> different paths: one that increments the induction value, and the
> other
> that uses the value not-yet-incremented.
> 
> If this is confusing, I can try to come up with something else.  The
> main point was to accomplish something like "an ascii picture is
> worth
> more than a few lines of comment".  :)

I figured that it was something like that, but it is less confusing with the pipes included. We don't charge by the line ;)

> 
> 
> > As far as I can tell, Hexagon has post-increment loads and stores.
> > This would not handle the case where the induction variable is a
> > pointer that is incremented using a post-increment load or store
> > (this applies to fixupInductionVariable as well).
> 
> Yes, this is a limitation.  This is something we may need to allow in
> the future, but for now could we leave it this way?  I'm not sure
> what
> the extent of changes would be to add this functionality, and I'd
> like
> to have it running for some time (i.e. tested) before submitting a
> patch
> here.

Addressing this later is fine. I'll need to add a similar functionality to the PPC version (to handle PPC pre-increment loads and stores).

Thanks again,
Hal

> 
> 
> > Thanks again,
> > Hal
> 
> Thank you.  I should have a patch with the comments addressed soon.
> 
> -Krzysztof
> 
> 
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>