[llvm-commits] Review request: Hexagon hardware loops

[Krzysztof Parzyszek]

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.


> +    /// 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".  :)


> 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.


> 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