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

[Stamatis Markianos-Wright <stam.markianos-wright@arm.com>]

On 06/11/2023 11:24, Richard Sandiford wrote:
> Stamatis Markianos-Wright <stam.markianos-wright@arm.com> writes:
>>> One of the main reasons for reading the arm bits was to try to answer
>>> the question: if we switch to a downcounting loop with a GE condition,
>>> how do we make sure that the start value is not a large unsigned
>>> number that is interpreted as negative by GE?  E.g. if the loop
>>> originally counted up in steps of N and used an LTU condition,
>>> it could stop at a value in the range [INT_MAX + 1, UINT_MAX].
>>> But the loop might never iterate if we start counting down from
>>> most values in that range.
>>>
>>> Does the patch handle that?
>> So AFAICT this is actually handled in the generic code in `doloop_valid_p`:
>>
>> This kind of loops fail because of they are "desc->infinite", then no
>> loop-doloop conversion is attempted at all (even for standard dls/le loops)
>>
>> Thanks to that check I haven't been able to trigger anything like the
>> behaviour you describe, do you think the doloop_valid_p checks are
>> robust enough?
> The loops I was thinking of are provably not infinite though.  E.g.:
>
>    for (unsigned int i = 0; i < UINT_MAX - 100; ++i)
>      ...
>
> is known to terminate.  And doloop conversion is safe with the normal
> count-down-by-1 approach, so I don't think current code would need
> to reject it.  I.e. a conversion to:
>
>    unsigned int i = UINT_MAX - 101;
>    do
>      ...
>    while (--i != ~0U);
>
> would be safe, but a conversion to:
>
>    int i = UINT_MAX - 101;
>    do
>      ...
>    while ((i -= step, i > 0));
>
> wouldn't, because the loop body would only be executed once.
>
> I'm only going off the name "infinite" though :)  It's possible that
> it has more connotations than that.
>
> Thanks,
> Richard

Ack, yep, I see what you mean now, and yep, that kind of loop does 
indeed pass through doloop_valid_p

Interestingly , in the v8-M Arm ARM this is done with:

```

boolean IsLastLowOverheadLoop(INSTR_EXEC_STATE_Type state)
// This does not check whether a loop is currently active.
// If the PE were in a loop, would this be the last one?
return UInt(state.LoopCount) <= (1 << (4 - LTPSIZE));

```

So architecturally the asm we output would be ok (except maybe the 
"branch too far subs;bgt;lctp" fallback at 
`predicated_doloop_end_internal` (maybe that should be `bhi`))... But 
now GE: isn't looking like an accurate representation of this operation 
in the compiler.

I'm wondering if I should try to make `predicated_doloop_end_internal` 
contain a comparison along the lines of:
(gtu: (plus: (LR) (const_int -num_lanes)) (const_int num_lanes_minus_1))

I'll give that a try :)

The only reason I'd chosen to go with GE earlier, tbh, was because of 
the existing handling of GE in loop-doloop.cc

Let me know if any other ideas come to your mind!


Cheers,

Stam