Re: [Qemu-devel] [PATCH v3 0/5] Add support for MSA instructions on a big endian host

Re: [Qemu-devel] [PATCH v3 0/5] Add support for MSA instructions on a big endian host

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 0/5] Add support for MSA instructions on a big endian host

"Add" -> "Fix"

Big endian host support should have worked from the moment MSA
support is added to QEMU.

>
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Add support for MSA instructions while executing QEMU on a
> machine that uses big endian MIPS CPU. Also change the
> implementation of helpers for MSA instructions
> ST.<B|H|W|D>, LD.<B|H|W|D>, INSERT.<B|H|W> (and D on MIPS64),
> COPY_S.<B|H|W> (and D on MIPS64) and COPY_U.<B|H> (and W
> on MIPS64). Instead of using a switch in a helper, which is
> called many times, put a switch in translate.c file, which
> is called only a few times.
> 

This paragraph suggests that there are essentially two independent
changes in this series (one for big endian host, another for refactoring
helpers), while in reality they are closely related. You should have
started the second sentence with "This is achieved by changing the
implementation of helpers for...".

> v3:
>  - Unroll loops in ST.<B|H|W|D> and LD.<B|H|W|D>
>    instructions.
>  - Eliminate macro that generates the helpers for
>    the ST.<B|H|W|D> and LD.<B|H|W|D> instructions,
>    and add four helpers for each (for byte,
>    halfword, word and doubleword).
>  - Eliminate the helpers for INSERT.<B|H|W|D>,
>    COPY_S.<B|H|W|D> and COPY_U.<B|H|W> and add
>    four (three in case of COPY_U) helpers for
>    each one.
> 

The cover letter should contain the complete series history.

Thanks,
Aleksandar

> Mateja Marjanovic (5):
>   target/mips: MSA instructions ld, big endian host fix
>   target/mips: MSA instructions st, big endian host fix
>   target/mips: Different approach toward COPY_S MSA instr. and big
>     endian fix
>   target/mips: Different approach toward COPY_U MSA instr. and big
>     endian fix
>   target/mips: Different approach toward INSERT MSA instr. and big
>     endian fix

Re: [Qemu-devel] [PATCH v3 1/5] target/mips: MSA instructions ld, big endian host fix

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 1/5] target/mips: MSA instructions ld, big endian host fix
> 

Start the title with the imperative, such as "target/mips: Fix..."

Use full instruction names, as explained in previous reviews.

> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Fix the case when the host is running on a big endian machine,
> and change the approach toward ld instruction helpers.
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>

Re: [Qemu-devel] [PATCH v3 2/5] target/mips: MSA instructions st, big endian host fix

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 2/5] target/mips: MSA instructions st, big endian host fix
>
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Fix the case when the host is running on a big endian machine,
> and change the approach toward st instruction helpers.
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---

The same comment as in the patch 1/5.

Also, please align both equation signs and argument indexes in the
new bodies of the helpers.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v3 3/5] target/mips: Different approach toward COPY_S MSA instr. and big endian fix

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 3/5] target/mips: Different approach toward COPY_S MSA instr. and big endian fix
> 

"Different approach toward..." -> "Refactor and fix COPY_S.<B|H|W|D> instructions"

> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> The old version of the helper for the COPY_S.<B|H|W|D> MSA instructions
> has been replaced with a four helpers that don't use switch, and change
> the endianness of the given index, when executed on a big endian host.
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---

Re: [Qemu-devel] [PATCH v3 4/5] target/mips: Different approach toward COPY_U MSA instr. and big endian fix

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 4/5] target/mips: Different approach toward COPY_U MSA instr. and big endian fix
>
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

"Different approach toward..." -> "Refactor and fix COPY_U.<B|H|W> instructions"

Re: [Qemu-devel] [PATCH v3 5/5] target/mips: Different approach toward INSERT MSA instr. and big endian fix

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v3 5/5] target/mips: Different approach toward INSERT MSA instr. and big endian fix

"Different approach toward..." -> "Refactor and fix INSERT.<B|H|W|D> instructions"

Re: [Qemu-devel] [PATCH v3 0/5] Add support for MSA instructions on a big endian host

[Mateja Marjanovic]

On 1.4.19. 19:53, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 0/5] Add support for MSA instructions on a big endian host
> "Add" -> "Fix"
>
> Big endian host support should have worked from the moment MSA
> support is added to QEMU.
I will change it in v4.
>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Add support for MSA instructions while executing QEMU on a
>> machine that uses big endian MIPS CPU. Also change the
>> implementation of helpers for MSA instructions
>> ST.<B|H|W|D>, LD.<B|H|W|D>, INSERT.<B|H|W> (and D on MIPS64),
>> COPY_S.<B|H|W> (and D on MIPS64) and COPY_U.<B|H> (and W
>> on MIPS64). Instead of using a switch in a helper, which is
>> called many times, put a switch in translate.c file, which
>> is called only a few times.
>>
> This paragraph suggests that there are essentially two independent
> changes in this series (one for big endian host, another for refactoring
> helpers), while in reality they are closely related. You should have
> started the second sentence with "This is achieved by changing the
> implementation of helpers for...".
I will make it more clear in v4.
>
>> v3:
>>   - Unroll loops in ST.<B|H|W|D> and LD.<B|H|W|D>
>>     instructions.
>>   - Eliminate macro that generates the helpers for
>>     the ST.<B|H|W|D> and LD.<B|H|W|D> instructions,
>>     and add four helpers for each (for byte,
>>     halfword, word and doubleword).
>>   - Eliminate the helpers for INSERT.<B|H|W|D>,
>>     COPY_S.<B|H|W|D> and COPY_U.<B|H|W> and add
>>     four (three in case of COPY_U) helpers for
>>     each one.
>>
> The cover letter should contain the complete series history.
I thought only the last version was necessary, but from now
on I will write the complete series history.
>
> Thanks,
> Aleksandar
>
>> Mateja Marjanovic (5):
>>    target/mips: MSA instructions ld, big endian host fix
>>    target/mips: MSA instructions st, big endian host fix
>>    target/mips: Different approach toward COPY_S MSA instr. and big
>>      endian fix
>>    target/mips: Different approach toward COPY_U MSA instr. and big
>>      endian fix
>>    target/mips: Different approach toward INSERT MSA instr. and big
>>      endian fix
Thanks,
Mateja

Re: [Qemu-devel] [PATCH v3 1/5] target/mips: MSA instructions ld, big endian host fix

[Mateja Marjanovic]

On 1.4.19. 19:56, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 1/5] target/mips: MSA instructions ld, big endian host fix
>>
> Start the title with the imperative, such as "target/mips: Fix..."
>
> Use full instruction names, as explained in previous reviews.
I will do that from now on.
>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Fix the case when the host is running on a big endian machine,
>> and change the approach toward ld instruction helpers.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
Thanks,
Mateja

Re: [Qemu-devel] [PATCH v3 2/5] target/mips: MSA instructions st, big endian host fix

[Mateja Marjanovic]

On 1.4.19. 19:58, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 2/5] target/mips: MSA instructions st, big endian host fix
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Fix the case when the host is running on a big endian machine,
>> and change the approach toward st instruction helpers.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> ---
> The same comment as in the patch 1/5.
>
> Also, please align both equation signs and argument indexes in the
> new bodies of the helpers.
It will be done in v4.
>
> Thanks,
> Aleksandar
Thanks,
Mateja

Re: [Qemu-devel] [PATCH v3 3/5] target/mips: Different approach toward COPY_S MSA instr. and big endian fix

[Mateja Marjanovic]

On 1.4.19. 20:01, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 3/5] target/mips: Different approach toward COPY_S MSA instr. and big endian fix
>>
> "Different approach toward..." -> "Refactor and fix COPY_S.<B|H|W|D> instructions"
I will change that in v4.
>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> The old version of the helper for the COPY_S.<B|H|W|D> MSA instructions
>> has been replaced with a four helpers that don't use switch, and change
>> the endianness of the given index, when executed on a big endian host.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> ---
Thanks,
Mateja

Re: [Qemu-devel] [PATCH v3 4/5] target/mips: Different approach toward COPY_U MSA instr. and big endian fix

[Mateja Marjanovic]

On 1.4.19. 20:02, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 4/5] target/mips: Different approach toward COPY_U MSA instr. and big endian fix
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> "Different approach toward..." -> "Refactor and fix COPY_U.<B|H|W> instructions"

Same goes for this, change in v4.

Thanks,
Mateja

Re: [Qemu-devel] [PATCH v3 5/5] target/mips: Different approach toward INSERT MSA instr. and big endian fix

[Mateja Marjanovic]

On 1.4.19. 20:03, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH v3 5/5] target/mips: Different approach toward INSERT MSA instr. and big endian fix
> "Different approach toward..." -> "Refactor and fix INSERT.<B|H|W|D> instructions"

Same goes for this, change in v4.

Thanks,
Mateja