[llvm-commits] Review request: Hexagon hardware loops

[Hal Finkel]

Krzysztof,

+    /// 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?

+        int diff = InstOffset - BlockToInstOffset[MII->getOperand(0).getMBB()];

This should be Diff (naming convention specifies a capital letter).
+    /// 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?

+  // Avoid linking errors in the non-optimized builds.
+  typedef HexagonHardwareLoops HWL;
+  const HWL::Comparison HWL::ComparisonKinds::EQ;
+  const HWL::Comparison HWL::ComparisonKinds::NE;
+  const HWL::Comparison HWL::ComparisonKinds::L;
...

Why not just makes these enum values?

+      bool isAdd = (UpdOpc == Hexagon::ADD_ri);
+
+      if (isAdd) {
+        // If the register operand to the add is the PHI we're
+        // looking at, this meets the induction pattern.

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

+        // Since the comparisons are "ri", the EndValue should be an
+        // immediate.  Check it just in case.
+        if (!EndValue->isImm())
           return 0;

Should this be an assert?

+    // The IV bump is a power of two.  Compute log_2, i.e. the shift amount.
+    unsigned Shift = 0;
+    while (IVBump >>= 1)
+      Shift++;

Can you use one of the Log2_* functions in MathExtras.h?

+  if (IndRegs.empty())
+    return false;
+
+
+  MachineBasicBlock *TB = 0, *FB = 0;

Extra newline? (there are a few of these).

+  // Verify that all existing predecessors, have analyzable branches
+  // (or no branches at all).

Extra comma in the comment.

Thanks again,
Hal

----- Original Message -----
> From: "Krzysztof Parzyszek" <kparzysz at codeaurora.org>
> To: llvm-commits at cs.uiuc.edu, "Hal Finkel" <hfinkel at anl.gov>
> Sent: Saturday, January 5, 2013 10:03:27 AM
> Subject: Review request: Hexagon hardware loops
> 
> This is a patch that significantly extends the functionality of the
> hardware loop generation on Hexagon.  Main changes are
> - separation of the loop fixup pass into its own file,
> - handling of nearly arbitrary comparison instructions in loop
> latches,
> - handling of nearly arbitrary lower and upper iteration bounds.
> 
> Please review.
> 
> Thank you,
> -Krzysztof
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>