Re: [PATCH v3 8/8] target/ppc: add vdiv{su}{wd} vmod{su}{wd} instructions

[Lijun Pan <ljp@linux.vnet.ibm.com>]

> On Jun 25, 2020, at 1:37 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 6/25/20 10:00 AM, Lijun Pan wrote:
>> +#define VDIV_MOD_DO(name, op, element, sign, bit)                       \
>> +    void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)       \
>> +    {                                                                   \
>> +        int i;                                                          \
>> +                                                                        \
>> +                                                                        \
>> +        for (i = 0; i < ARRAY_SIZE(r->element); i++) {                  \
>> +            if (unlikely((b->element[i] == 0) ||                        \
>> +                (sign &&                                                \
>> +                (b->element[i] == UINT##bit##_MAX) &&                   \
>> +                (a->element[i] == INT##bit##_MIN))))                    \
>> +                continue;                                               \
>> +            r->element[i] = a->element[i] op b->element[i];             \
>> +        }                                                               \
>> +    }
> 
> Missing braces for the if.  Extra blank line before the for.

No, the braces are enough. "unlikely" is to describe the whole logic,
eg. if (unlikely( (divisor == 0) || (sign && (divisor == 0xFFFFFFFF) && (dividend = 0x80000000) ) ) )

I will remove that blank line.

> 
> I see that the ISA document says divide-by-zero produces an undefined result,
> so leaving the previous contents does seem to be within the letter of the law.
> 
> However... Are you able to test what real hardware produces?  It would be nice
> (but not required) to match if it is simple to do so.
> 
> Whichever way we go with the undefined result, this deserves a comment.

I will add “continue; / * Undefined, No Special Registers Altered */ "

Lijun