Re: [PATCH] Fix result for conditional reductions matching at index 0

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Nov 22, 2017 at 12:01 PM, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
>> On 22 Nov 2017, at 09:14, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>> On Tue, Nov 21, 2017 at 5:43 PM, Kilian Verhetsel
>> <kilian.verhetsel@uclouvain.be> wrote:
>>>
>>>> This is PR81179 I think, please mention that in the changelog.
>>>
>>> Correct, my bad for missing that.
>>>
>>>> This unconditionally pessimizes code even if there is no valid index
>>>> zero, right?
>>>
>>> Almost, since for a loop such as:
>>>
>>>  #define OFFSET 1
>>>  unsigned int find(const unsigned int *a, unsigned int v) {
>>>    unsigned int result = 120;
>>>    for (unsigned int i = OFFSET; i < 32+OFFSET; i++) {
>>>      if (a[i-OFFSET] == v) result = i;
>>>    }
>>>    return result;
>>>  }
>>>
>>> the index i will match the contents of the index vector used here ---
>>> but this does indeed pessimize the code generated for, say, OFFSET
>>> = 2. It is probably more sensible to use the existing code path in those
>>> situations.
>>>
>>>> The issue with the COND_REDUCITION index vector is overflow IIRC.
>>>
>>> Does that mean such overflows can already manifest themselves for
>>> regular COND_REDUCTION? I had assumed sufficient checks were already in
>>> place because of the presence of the is_nonwrapping_integer_induction
>>> test.
>>
>> But only if we need the index vector?  The patch looked like you're changing
>> how other modes are handled (in my look I didn't make myself familiar with
>> the various modes again...).  Anyway, Alan hopefully remembers what he
>> coded so I'll defer to him here.
>>
>> If Alan is happy with the patch consider it approved.
>>
>
> Richard’s right with his question.
>
> The optimisation needs to fail if the number of interactions of the loop + 1 doesn’t
> fit into the data type used for the result.
>
> I took the test pr65947-14.c
> First I set N to 0xffffffff-1. This compiled and vectorised. That’s ok.
> Now if I set N to 0xffffffff it still vectorises, but this should fail.
>
> Compare to pr65947-14.c where we set  last = a[I]; inside the if.
> When set N to 0xffffffff-1, it compiled and vectorised. That’s ok.
> When set N to 0xffffffff it fails to vectorise with the message
> "loop size is greater than data size”.
>
> Looks like you might just need to add the one check.
>
> Also see pr65947-9.c which uses the slightly more useful char indexes.

Some extra test variants might be indeed useful to really cover all corner
cases.  The PR had some "trivial" patch for the issue by me that I considered
a too big hammer.

I wonder if other compilers pull another trick to deal with the situation.

Richard.

>
> Alan.
>
>
>