Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops

[Andre Simoes Dias Vieira <Andre.SimoesDiasVieira@arm.com>]

Hi Richard,

Thanks for the reviews, I'm making these changes but just a heads up.

When hardcoding LR_REGNUM like this we need to change the way we compare the register in doloop_condition_get. This function currently compares the rtx nodes by address, which I think happens to be fine before we assign hard registers, as I suspect we always share the rtx node for the same pseudo, but when assigning registers it seems like we create copies, so things like:
`XEXP (inc_src, 0) == reg` will fail for
inc_src: (plus (reg LR) (const_int -n)'
reg: (reg LR)

Instead I will substitute the operand '==' with calls to 'rtx_equal_p (op1, op2, NULL)'.

Sound good?

Kind regards,
Andre

________________________________________
From: Richard Earnshaw (lists) <Richard.Earnshaw@arm.com>
Sent: Tuesday, January 30, 2024 11:36 AM
To: Andre Simoes Dias Vieira; gcc-patches@gcc.gnu.org
Cc: Kyrylo Tkachov; Stam Markianos-Wright
Subject: Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops

On 19/01/2024 14:40, Andre Vieira wrote:
>
> Respin after comments from Kyrill and rebase. I also removed an if-then-else
> construct in arm_mve_check_reg_origin_is_num_elems similar to the other functions
> Kyrill pointed out.
>
> After an earlier comment from Richard Sandiford I also added comments to the
> two tail predication patterns added to explain the need for the unspecs.

[missing ChangeLog]

I'm just going to focus on loop-doloop.c in this reply, I'll respond to the other bits in a follow-up.

      2)  (set (reg) (plus (reg) (const_int -1))
-         (set (pc) (if_then_else (reg != 0)
-                                (label_ref (label))
-                                (pc))).
+        (set (pc) (if_then_else (reg != 0)
+                                (label_ref (label))
+                                (pc))).

      Some targets (ARM) do the comparison before the branch, as in the
      following form:

-     3) (parallel [(set (cc) (compare ((plus (reg) (const_int -1), 0)))
-                   (set (reg) (plus (reg) (const_int -1)))])
-        (set (pc) (if_then_else (cc == NE)
...


This comment is becoming confusing.  Really the text leading up to 3)... should be inside 3.  Something like:

      3) Some targets (ARM) do the comparison before the branch, as in the
      following form:

      (parallel [(set (cc) (compare (plus (reg) (const_int -1)) 0))
                 (set (reg) (plus (reg) (const_int -1)))])
      (set (pc) (if_then_else (cc == NE)
                              (label_ref (label))
                              (pc)))])


The same issue on the comment structure also applies to the new point 4...

+      The ARM target also supports a special case of a counter that decrements
+      by `n` and terminating in a GTU condition.  In that case, the compare and
+      branch are all part of one insn, containing an UNSPEC:
+
+      4) (parallel [
+           (set (pc)
+               (if_then_else (gtu (unspec:SI [(plus:SI (reg:SI 14 lr)
+                                                       (const_int -n))])
+                                  (const_int n-1]))
+                   (label_ref)
+                   (pc)))
+           (set (reg:SI 14 lr)
+                (plus:SI (reg:SI 14 lr)
+                         (const_int -n)))
+     */

I think this needs a bit more clarification.  Specifically that this construct supports a predicated vectorized do loop.  Also, the placement of the unspec inside the comparison is ugnly and unnecessary.  It should be sufficient to have the unspec inside a USE expression, which the mid-end can then ignore entirely.  So

        (parallel
         [(set (pc) (if_then_else (gtu (plus (reg) (const_int -n))
                                       (const_int n-1))
                                  (label_ref) (pc)))
          (set (reg) (plus (reg) (const_int -n)))
          (additional clobbers and uses)])

For Arm, we then add a (use (unspec [(const_int 0)] N)) that is specific to this pattern to stop anything else from matching it.

Note that we don't need to mention that the register is 'LR' or the modes, those are specific to a particular backend, not the generic pattern we want to match.

+      || !CONST_INT_P (XEXP (inc_src, 1))
+      || INTVAL (XEXP (inc_src, 1)) >= 0)
     return 0;
+  int dec_num = abs (INTVAL (XEXP (inc_src, 1)));

We can just use '-INTVAL(...)' here, we've verified just above that the constant is negative.

-  if ((XEXP (condition, 0) == reg)
+  /* For the ARM special case of having a GTU: re-form the condition without
+     the unspec for the benefit of the middle-end.  */
+  if (GET_CODE (condition) == GTU)
+    {
+      condition = gen_rtx_fmt_ee (GTU, VOIDmode, inc_src,
+                                 GEN_INT (dec_num - 1));
+      return condition;
+    }

If you make the change I mentioned above, this re-forming isn't needed any more, so the arm-specific comment goes away

-   {
+    {
      if (GET_CODE (pattern) != PARALLEL)
      /*  For the second form we expect:

You've fixed the indentation of the brace (good), but the body of the braced expression needs re-indenting as well.

R.