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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>  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.

A second more problematic issue is

exp ? one
    : two

because all indentation styles I'm aware of would move the second line
to left.  This one can't be fixed in any reasonable way by adding
brackets if there were previously none, because even if it was
bracketed, the else case would still move left.

>
>> 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.

~Andrew

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