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

[David Miller]

Makes sense,  thanks for the quick reply.
Last question,  the patches can depend on others in the same set right?
IE:  all of the additions to insn-data.def in one, implementations in separate patches.

Thanks

- David Miller

On Thu, Mar 3, 2022 at 12:42 PM Richard Henderson <richard.henderson@linaro.org> wrote:

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~