| Message ID | 20190709184823.4135-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg/aarch64: Fix output of extract2 opcodes | expand |
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 > >
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 > > > >
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 > > > > > >
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>
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
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
> Can you be less combative in your review comments, please?
Sure, I will.
Thanks, Aleksandar
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 > >
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~
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~
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:
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(-) -- 2.17.1