Re: [Qemu-devel] [PATCH 12/38] tcg: Add gvec expanders for variable shift

[David Hildenbrand]

On 23.04.19 23:40, Richard Henderson wrote:
> On 4/23/19 2:02 PM, David Hildenbrand wrote:
>> On 23.04.19 21:28, Richard Henderson wrote:
>>> On 4/23/19 12:04 PM, David Hildenbrand wrote:
>>>> In order to use this on s390x for VECTOR ELEMENT SHIFT, like
>>>>
>>>> +static DisasJumpType op_vesv(DisasContext *s, DisasOps *o)
>>>> +{
>>>> +    const uint8_t es = get_field(s->fields, m4);
>>>> +    const uint8_t v1 = get_field(s->fields, v1);
>>>> +    const uint8_t v2 = get_field(s->fields, v2);
>>>> +    const uint8_t v3 = get_field(s->fields, v3);
>>>> +
>>>> +    if (es > ES_64) {
>>>> +        gen_program_exception(s, PGM_SPECIFICATION);
>>>> +        return DISAS_NORETURN;
>>>> +    }
>>>> +
>>>> +    switch (s->fields->op2) {
>>>> +    case 0x70:
>>>> +        gen_gvec_fn_3(shlv, es, v1, v2, v3);
>>>> +        break;
>>>> +    case 0x7a:
>>>> +        gen_gvec_fn_3(sarv, es, v1, v2, v3);
>>>> +        break;
>>>> +    case 0x78:
>>>> +        gen_gvec_fn_3(shrv, es, v1, v2, v3);
>>>> +        break;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +
>>>> +    return DISAS_NEXT;
>>>> +}
>>>>
>>>> We need to mask of invalid bits from the shift. Can that be added?
>>>
>>> Yes, I do exactly this in patch 31 for target/ppc.
>>>
>>>
>>> r~
>>>
>>
>> Got it, so not via a generic gvec expansion. Thanks!
> 
> I do wonder if I *should* make the truncating expansion the
> generic gvec expansion.  It would be usable from two targets
> at least...
> 
> Because the one's that *don't* truncate in hardware will
> still have to have their own custom expansion code.
> 
> Thoughts?
> 

Exactly what I had in mind. I wonder if the same applies to all shift
helpers/expansions. Expected behavior (mask off bits) is always better
than unexpected behavior. But of course I might be wrong. At least, here
I would vote for doing the truncation in the generic gvec expansion.

> 
> r~
> 


-- 

Thanks,

David / dhildenb