[Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Richard Henderson]

The aarch64 argument ordering for the operands is big-endian,
whereas the tcg argument ordering is little-endian.  Use REG0
so that we honor the rZ constraints.

Fixes: 464c2969d5d
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/aarch64/tcg-target.inc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index b0f8106642..0713448bf5 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
 
     case INDEX_op_extract2_i64:
     case INDEX_op_extract2_i32:
-        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
+        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
         break;
 
     case INDEX_op_add2_i32:
-- 
2.17.1

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Aleksandar Markovic]

On Jul 9, 2019 8:56 PM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>
> The aarch64 argument ordering for the operands is big-endian,
> whereas the tcg argument ordering is little-endian.  Use REG0
> so that we honor the rZ constraints.
>
> Fixes: 464c2969d5d
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

The commit message looks more like a list of some random items than logical
explanation of the code change. Improve it.

Regards,
Aleksandar

>  tcg/aarch64/tcg-target.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index b0f8106642..0713448bf5 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>
>      case INDEX_op_extract2_i64:
>      case INDEX_op_extract2_i32:
> -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
>          break;
>
>      case INDEX_op_add2_i32:
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Richard Henderson]

On Wed, 10 Jul 2019 at 11:22, Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> On Jul 9, 2019 8:56 PM, "Richard Henderson" <richard.henderson@linaro.org> wrote:
> >
> > The aarch64 argument ordering for the operands is big-endian,
> > whereas the tcg argument ordering is little-endian.  Use REG0
> > so that we honor the rZ constraints.
> >
> > Fixes: 464c2969d5d
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
>
> The commit message looks more like a list of some random items than logical explanation of the code change. Improve it.

Vague and non-constructive comments like this are and will continue to
be ignored.

If you want to review a patch, then you're going to have to actually
read it.  There are two obvious changes in the one line patch.  Each
sentence describes the reason for each change.  There is no subtle
complex problem here.

r~

>
> Regards,
> Aleksandar
>
> >  tcg/aarch64/tcg-target.inc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> > index b0f8106642..0713448bf5 100644
> > --- a/tcg/aarch64/tcg-target.inc.c
> > +++ b/tcg/aarch64/tcg-target.inc.c
> > @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> >
> >      case INDEX_op_extract2_i64:
> >      case INDEX_op_extract2_i32:
> > -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> > +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
> >          break;
> >
> >      case INDEX_op_add2_i32:
> > --
> > 2.17.1
> >
> >

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Aleksandar Markovic]

On Jul 10, 2019 11:48 AM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>
> On Wed, 10 Jul 2019 at 11:22, Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> > On Jul 9, 2019 8:56 PM, "Richard Henderson" <
richard.henderson@linaro.org> wrote:
> > >
> > > The aarch64 argument ordering for the operands is big-endian,
> > > whereas the tcg argument ordering is little-endian.  Use REG0
> > > so that we honor the rZ constraints.
> > >
> > > Fixes: 464c2969d5d
> > > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> >
> > The commit message looks more like a list of some random items than
logical explanation of the code change. Improve it.
>
> Vague and non-constructive comments like this are and will continue to
> be ignored.
>

You can continue ignoring any comment that you don't like, as you already
have been doing for long time, but this will certainly not improve your
code, and is contrary to the spirit of open source.

Regards, Aleksandar

> If you want to review a patch, then you're going to have to actually
> read it.  There are two obvious changes in the one line patch.  Each
> sentence describes the reason for each change.  There is no subtle
> complex problem here.
>
> r~
>
> >
> > Regards,
> > Aleksandar
> >
> > >  tcg/aarch64/tcg-target.inc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tcg/aarch64/tcg-target.inc.c
b/tcg/aarch64/tcg-target.inc.c
> > > index b0f8106642..0713448bf5 100644
> > > --- a/tcg/aarch64/tcg-target.inc.c
> > > +++ b/tcg/aarch64/tcg-target.inc.c
> > > @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode
opc,
> > >
> > >      case INDEX_op_extract2_i64:
> > >      case INDEX_op_extract2_i32:
> > > -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> > > +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
> > >          break;
> > >
> > >      case INDEX_op_add2_i32:
> > > --
> > > 2.17.1
> > >
> > >

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Peter Maydell]

On Wed, 10 Jul 2019 at 10:22, Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> The commit message looks more like a list of some random items
> than logical explanation of the code change. Improve it.

Can you be less combative in your review comments, please?
Providing constructive and specific suggestions for the
improvements you'd like to see is more likely to help
us produce better software than abrupt orders to "improve it".

This is about the third or fourth time you've done this
with RTH's patches and I think it is not really warranted.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Aleksandar Markovic]

> Can you be less combative in your review comments, please?

Sure, I will.

Thanks, Aleksandar

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Philippe Mathieu-Daudé]

On 7/9/19 8:48 PM, Richard Henderson wrote:
> The aarch64 argument ordering for the operands is big-endian,
> whereas the tcg argument ordering is little-endian.  Use REG0
> so that we honor the rZ constraints.
> 
> Fixes: 464c2969d5d
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/aarch64/tcg-target.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index b0f8106642..0713448bf5 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  
>      case INDEX_op_extract2_i64:
>      case INDEX_op_extract2_i32:
> -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
>          break;
>  
>      case INDEX_op_add2_i32:
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The aarch64 argument ordering for the operands is big-endian,
> whereas the tcg argument ordering is little-endian.  Use REG0
> so that we honor the rZ constraints.
>
> Fixes: 464c2969d5d
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

I ran a bunch of AArch64 EXTR testcases on AArch64 and hit the code at
least 4600 times ;-)

Tested-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tcg/aarch64/tcg-target.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index b0f8106642..0713448bf5 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>
>      case INDEX_op_extract2_i64:
>      case INDEX_op_extract2_i32:
> -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
>          break;
>
>      case INDEX_op_add2_i32:


--
Alex Bennée

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Aleksandar Markovic]

On Tue, Jul 9, 2019 at 8:56 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The aarch64 argument ordering for the operands is big-endian,
> whereas the tcg argument ordering is little-endian.  Use REG0
> so that we honor the rZ constraints.

Hello, Richard.

If endian and rZ constraints are unrelated problems, then I think the
commit message
should be:

"This patch fixes two problem:

- endianness: the aarch64 argument ordering for the operands is
big-endian, whereas the tcg argument ordering is little-endian.

- rZ constrains: REG0() macro should be applied to the affected
arguments."

One could argue that in this case the patch this should be actually two patches.
This is better because of bisectability. The number of line in the
patch doesn't matter.

If, on the other hand, endian and rZ constraints are related problems, then you
should explain how in the commit message.

In general, your commit messages appear too terse, and it is hard to establish
logical sense of the changes in question.

Would you be so kind to consider my opinion?

Regards,
Aleksandar

>
> Fixes: 464c2969d5d
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/aarch64/tcg-target.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index b0f8106642..0713448bf5 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -2226,7 +2226,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>
>      case INDEX_op_extract2_i64:
>      case INDEX_op_extract2_i32:
> -        tcg_out_extr(s, ext, a0, a1, a2, args[3]);
> +        tcg_out_extr(s, ext, a0, REG0(2), REG0(1), args[3]);
>          break;
>
>      case INDEX_op_add2_i32:
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Richard Henderson]

On 7/10/19 8:12 PM, Aleksandar Markovic wrote:
> On Tue, Jul 9, 2019 at 8:56 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> The aarch64 argument ordering for the operands is big-endian,
>> whereas the tcg argument ordering is little-endian.  Use REG0
>> so that we honor the rZ constraints.
> 
> Hello, Richard.
> 
> If endian and rZ constraints are unrelated problems, then I think the
> commit message
> should be:
> 
> "This patch fixes two problem:
> 
> - endianness: the aarch64 argument ordering for the operands is
> big-endian, whereas the tcg argument ordering is little-endian.
> 
> - rZ constrains: REG0() macro should be applied to the affected
> arguments."

That's fair.

> One could argue that in this case the patch this should be actually two patches.
> This is better because of bisectability. The number of line in the
> patch doesn't matter.

Well, nothing between the faulty commit (Fixes: 464c2969d5d) and the second of
the two prospective patches is really bisectable.  For the given test case, all
points in between will fail at runtime, even if each point compiles.

Therefore I don't see the point in separating the two changes.

> Would you be so kind to consider my opinion?

Yes.  Thanks for the expanded opinion.

I plan to change the commit message to:

---
tcg/aarch64: Fix output of extract2 opcodes

This patch fixes two problems:
(1) The inputs to the EXTR insn were reversed,
(2) The input constraints use rZ, which means that we need to use
    the REG0 macro in order to supply XZR for a constant 0 input.

r-b, s-o-b, etc
---

Does that seem sufficient to you?


r~

Re: [Qemu-devel] [PATCH] tcg/aarch64: Fix output of extract2 opcodes

[Aleksandar Markovic]

On Thu, Jul 11, 2019 at 1:45 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/10/19 8:12 PM, Aleksandar Markovic wrote:
> > On Tue, Jul 9, 2019 at 8:56 PM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> The aarch64 argument ordering for the operands is big-endian,
> >> whereas the tcg argument ordering is little-endian.  Use REG0
> >> so that we honor the rZ constraints.
> >
> > Hello, Richard.
> >
> > If endian and rZ constraints are unrelated problems, then I think the
> > commit message
> > should be:
> >
> > "This patch fixes two problem:
> >
> > - endianness: the aarch64 argument ordering for the operands is
> > big-endian, whereas the tcg argument ordering is little-endian.
> >
> > - rZ constrains: REG0() macro should be applied to the affected
> > arguments."
>
> That's fair.
>
> > One could argue that in this case the patch this should be actually two patches.
> > This is better because of bisectability. The number of line in the
> > patch doesn't matter.
>
> Well, nothing between the faulty commit (Fixes: 464c2969d5d) and the second of
> the two prospective patches is really bisectable.  For the given test case, all
> points in between will fail at runtime, even if each point compiles.
>
> Therefore I don't see the point in separating the two changes.
>
> > Would you be so kind to consider my opinion?
>
> Yes.  Thanks for the expanded opinion.
>
> I plan to change the commit message to:
>
> ---
> tcg/aarch64: Fix output of extract2 opcodes
>
> This patch fixes two problems:
> (1) The inputs to the EXTR insn were reversed,
> (2) The input constraints use rZ, which means that we need to use
>     the REG0 macro in order to supply XZR for a constant 0 input.
>
> r-b, s-o-b, etc
> ---
>
> Does that seem sufficient to you?
>

It does. That's super. Thank you very much!

Yours,
Aleksandar

>
> r~