Re: [Xen-devel] [PATCH v7 06/49] x86emul: support AVX512{F, BW, DQ} integer broadcast insns

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.19 at 17:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/03/2019 17:15, Jan Beulich wrote:
>>>>> On 14.03.19 at 17:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 19/12/2018 14:38, Jan Beulich wrote:
>>>> @@ -8444,6 +8465,45 @@ x86_emulate(
>>>>          generate_exception_if(ea.type != OP_MEM || !vex.l || vex.w, 
>>>> EXC_UD);
>>>>          goto simd_0f_avx2;
>>>>  
>>>> +    case X86EMUL_OPC_EVEX_66(0x0f38, 0x78): /* vpbroadcastb 
>>>> xmm/m8,[xyz]mm{k} */
>>>> +    case X86EMUL_OPC_EVEX_66(0x0f38, 0x79): /* vpbroadcastw 
>>>> xmm/m16,[xyz]mm{k} */
>>>> +        host_and_vcpu_must_have(avx512bw);
>>>> +        generate_exception_if(evex.w || evex.brs, EXC_UD);
>>>> +        op_bytes = elem_bytes = 1 << (b & 1);
>>>> +        /* See the comment at the avx512_broadcast label. */
>>>> +        op_mask |= !(b & 1 ? !(uint32_t)op_mask : !op_mask);
>>>> +        goto avx512f_no_sae;
>>>> +
>>>> +    case X86EMUL_OPC_EVEX_66(0x0f38, 0x7a): /* vpbroadcastb 
>>>> r32,[xyz]mm{k} */
>>>> +    case X86EMUL_OPC_EVEX_66(0x0f38, 0x7b): /* vpbroadcastw 
>>>> r32,[xyz]mm{k} */
>>>> +        host_and_vcpu_must_have(avx512bw);
>>>> +        generate_exception_if(evex.w, EXC_UD);
>>>> +        /* fall through */
>>>> +    case X86EMUL_OPC_EVEX_66(0x0f38, 0x7c): /* vpbroadcast{d,q} 
>>>> reg,[xyz]mm{k} */
>>>> +        generate_exception_if((ea.type != OP_REG || evex.brs ||
>>>> +                               evex.reg != 0xf || !evex.RX),
>>>> +                              EXC_UD);
>>> generate_exception_if(ea.type != OP_REG || evex.brs ||
>>>                       evex.reg != 0xf || !evex.RX, EXC_UD);
>>>
>>> ?
>> Well, no - I don't really want the second argument on a continued
>> line of the first one.
> 
> Why not?  We do this elsewhere, and it doesn't help readability to split
> them.

Well, readability is always a subjective thing. To me, doing what
you suggest would result in an increased risk of no realizing that
there's a second argument at the end of the second line (granted
it's not a bad here, but it would get quite bad with more than two
arguments, and a mixture of splitting and adding further ones on
the continued arguments' lines).

>>  Multiple full arguments on one line are fine
>> with me. If you want me to drop just the inner parentheses, that
>> would be fine (as mentioned in another context on this series, I've
>> mainly added them because of what I understood your editor's
>> behavior is when splitting lines like this, and I may have easily
>> misunderstood).
> 
> Its only when your preferred indentation deviates from BSD style.  If
> there are already brackets around, the chances are its fine, because the
> align with brackets rule takes over.
> 
> The main issue is with return statements, where BSD style would require
> the continuation of the statement to start under the second r of return.

Ah, I see. So in the case above the inner parentheses aren't
really necessary from the editor's pov. I still think they're useful
to make visually clear at the first glance the difference between
continued arguments and the start of further ones.

>>> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Let me know on what variant(s) of the above this holds.
> 
> I'd prefer not to split EXC_UD onto a line of its own, because I don't
> think it helps the readability of the surrounding code, but I'm not
> sufficiently bothered to make it a hard requirement.

Thanks.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel