[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Richard Henderson
Subject: Re: [PATCH v1 1/2] s390x/tcg: Implement Vector-Enhancements Facility 2 for s390x
Date: Thu, 3 Mar 2022 07:42:46 -1000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]