Re: [PATCH v1 1/2] s390x/tcg: Implement Vector-Enhancements Facility 2 for s390x

[Richard Henderson]

On 3/3/22 06:50, David Miller wrote:
>  > Too many changes in one patch.
>  > You need to split these into smaller, logical units.
>
> Can you give some guideline on that?
> IE: change to two,  the shifts and reversed loads into two patches or more on line count 
> of each patch?

Your best guide is line count: < 50 is ideal, though of course that can't always be done. 
 For bug fixes or code reorg you may find yourself constrained by not breaking bisection.

But for new code, like this, one patch per feature is easiest to review.  In this case 
you've got:

  - load/store elements reversed,
  - load/store byte reversed elements,
  - shift double
  - string search
  - modify fp convert
  - modify shift

> I wasn't sure if there was a reason MO_TE was used so just kept with the 
> existing code flow.

We have to put some indication of endianness there, and "target" endian was the easiest to 
replicate across all targets.  Especially with those that are bi-endian.

I've just noticed that we haven't propagated this to the integer load/store reversed.  I 
presume that code pre-dates the existence of the feature.  But it would be good to change

    C(0xe31f, LRVH,    RXY_a, Z,   0, m2_16u, new, r1_16, rev16, 0)
    C(0xe31e, LRV,     RXY_a, Z,   0, m2_32u, new, r1_32, rev32, 0)
    C(0xe30f, LRVG,    RXY_a, Z,   0, m2_64, r1, 0, rev64, 0)
...
    C(0xe33f, STRVH,   RXY_a, Z,   la2, r1_16u, new, m1_16, rev16, 0)
    C(0xe33e, STRV,    RXY_a, Z,   la2, r1_32u, new, m1_32, rev32, 0)
    C(0xe32f, STRVG,   RXY_a, Z,   la2, r1_o, new, m1_64, rev64, 0)

to use little-endian memory ops, rather than separately reversing the bytes.


r~