| Message ID | 20190815020928.9679-3-jan.bobek@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rewrite MMX/SSE/SSE2/SSE3 instruction translation | expand |
15.08.2019. 04.13, "Jan Bobek" <jan.bobek@gmail.com> је написао/ла: > > From: Richard Henderson <rth@twiddle.net> > > Treat this the same as we already do for other rex bits. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/i386/translate.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/target/i386/translate.c b/target/i386/translate.c > index d74dbfd585..c0866c2797 100644 > --- a/target/i386/translate.c > +++ b/target/i386/translate.c > @@ -44,11 +44,13 @@ > #define REX_X(s) ((s)->rex_x) > #define REX_B(s) ((s)->rex_b) > #define REX_R(s) ((s)->rex_r) > +#define REX_W(s) ((s)->rex_w) > #else > #define CODE64(s) 0 > #define REX_X(s) 0 > #define REX_B(s) 0 > #define REX_R(s) 0 > +#define REX_W(s) -1 The commit message says "treat rex_w the same as other rex bits". Why is then REX_W() treated differently here? Yours, Aleksandar > #endif > > #ifdef TARGET_X86_64 > @@ -100,7 +102,7 @@ typedef struct DisasContext { > #ifdef TARGET_X86_64 > int lma; /* long mode active */ > int code64; /* 64 bit code segment */ > - int rex_x, rex_b, rex_r; > + int rex_x, rex_b, rex_r, rex_w; > #endif > int vex_l; /* vex vector length */ > int vex_v; /* vex vvvv register, without 1's complement. */ > @@ -4495,7 +4497,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > int modrm, reg, rm, mod, op, opreg, val; > target_ulong next_eip, tval; > target_ulong pc_start = s->base.pc_next; > - int rex_w; > > s->pc_start = s->pc = pc_start; > s->override = -1; > @@ -4503,6 +4504,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > s->rex_x = 0; > s->rex_b = 0; > s->rex_r = 0; > + s->rex_w = -1; > s->x86_64_hregs = false; > #endif > s->rip_offset = 0; /* for relative ip address */ > @@ -4514,7 +4516,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > } > > prefixes = 0; > - rex_w = -1; > > next_byte: > b = x86_ldub_code(env, s); > @@ -4557,7 +4558,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > case 0x40 ... 0x4f: > if (CODE64(s)) { > /* REX prefix */ > - rex_w = (b >> 3) & 1; > + s->rex_w = (b >> 3) & 1; > s->rex_r = (b & 0x4) << 1; > s->rex_x = (b & 0x2) << 2; > s->rex_b = (b & 0x1) << 3; > @@ -4606,7 +4607,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > s->rex_b = (~vex2 >> 2) & 8; > #endif > vex3 = x86_ldub_code(env, s); > - rex_w = (vex3 >> 7) & 1; > +#ifdef TARGET_X86_64 > + s->rex_w = (vex3 >> 7) & 1; > +#endif > switch (vex2 & 0x1f) { > case 0x01: /* Implied 0f leading opcode bytes. */ > b = x86_ldub_code(env, s) | 0x100; > @@ -4631,9 +4634,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > /* Post-process prefixes. */ > if (CODE64(s)) { > /* In 64-bit mode, the default data size is 32-bit. Select 64-bit > - data with rex_w, and 16-bit data with 0x66; rex_w takes precedence > + data with REX_W, and 16-bit data with 0x66; REX_W takes precedence > over 0x66 if both are present. */ > - dflag = (rex_w > 0 ? MO_64 : prefixes & PREFIX_DATA ? MO_16 : MO_32); > + dflag = (REX_W(s) > 0 ? MO_64 : prefixes & PREFIX_DATA ? MO_16 : MO_32); > /* In 64-bit mode, 0x67 selects 32-bit addressing. */ > aflag = (prefixes & PREFIX_ADR ? MO_32 : MO_64); > } else { > @@ -5029,7 +5032,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > /* operand size for jumps is 64 bit */ > ot = MO_64; > } else if (op == 3 || op == 5) { > - ot = dflag != MO_16 ? MO_32 + (rex_w == 1) : MO_16; > + ot = dflag != MO_16 ? MO_32 + (REX_W(s) == 1) : MO_16; > } else if (op == 6) { > /* default push size is 64 bit */ > ot = mo_pushpop(s, dflag); > -- > 2.20.1 > >
On 8/15/19 8:30 AM, Aleksandar Markovic wrote: > > 15.08.2019. 04.13, "Jan Bobek" <jan.bobek@gmail.com > <mailto:jan.bobek@gmail.com>> је написао/ла: >> >> From: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net>> >> >> Treat this the same as we already do for other rex bits. >> >> Signed-off-by: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net>> >> --- >> target/i386/translate.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/target/i386/translate.c b/target/i386/translate.c >> index d74dbfd585..c0866c2797 100644 >> --- a/target/i386/translate.c >> +++ b/target/i386/translate.c >> @@ -44,11 +44,13 @@ >> #define REX_X(s) ((s)->rex_x) >> #define REX_B(s) ((s)->rex_b) >> #define REX_R(s) ((s)->rex_r) >> +#define REX_W(s) ((s)->rex_w) >> #else >> #define CODE64(s) 0 >> #define REX_X(s) 0 >> #define REX_B(s) 0 >> #define REX_R(s) 0 >> +#define REX_W(s) -1 > > The commit message says "treat rex_w the same as other rex bits". Why is then > REX_W() treated differently here? "Treated the same" in terms of being referenced by a macro instead of a local variable. As for the -1, if you look at the rest of the patch you can clearly see it preserves existing behaviour. >> @@ -4503,6 +4504,7 @@ static target_ulong disas_insn(DisasContext *s, > CPUState *cpu) >> s->rex_x = 0; >> s->rex_b = 0; >> s->rex_r = 0; >> + s->rex_w = -1; >> s->x86_64_hregs = false; >> #endif >> s->rip_offset = 0; /* for relative ip address */ >> @@ -4514,7 +4516,6 @@ static target_ulong disas_insn(DisasContext *s, > CPUState *cpu) >> } >> >> prefixes = 0; >> - rex_w = -1; r~
15.08.2019. 11.55, "Richard Henderson" <richard.henderson@linaro.org> је написао/ла: > > On 8/15/19 8:30 AM, Aleksandar Markovic wrote: > > > > 15.08.2019. 04.13, "Jan Bobek" <jan.bobek@gmail.com > > <mailto:jan.bobek@gmail.com>> је написао/ла: > >> > >> From: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net>> > >> > >> Treat this the same as we already do for other rex bits. > >> > >> Signed-off-by: Richard Henderson <rth@twiddle.net <mailto: rth@twiddle.net>> > >> --- > >> target/i386/translate.c | 19 +++++++++++-------- > >> 1 file changed, 11 insertions(+), 8 deletions(-) > >> > >> diff --git a/target/i386/translate.c b/target/i386/translate.c > >> index d74dbfd585..c0866c2797 100644 > >> --- a/target/i386/translate.c > >> +++ b/target/i386/translate.c > >> @@ -44,11 +44,13 @@ > >> #define REX_X(s) ((s)->rex_x) > >> #define REX_B(s) ((s)->rex_b) > >> #define REX_R(s) ((s)->rex_r) > >> +#define REX_W(s) ((s)->rex_w) > >> #else > >> #define CODE64(s) 0 > >> #define REX_X(s) 0 > >> #define REX_B(s) 0 > >> #define REX_R(s) 0 > >> +#define REX_W(s) -1 > > > > The commit message says "treat rex_w the same as other rex bits". Why is then > > REX_W() treated differently here? > > "Treated the same" in terms of being referenced by a macro instead of a local > variable. As for the -1, if you look at the rest of the patch you can clearly > see it preserves existing behaviour. > That is exactly what I dislike about your commit messages: they often introduce ambiguity, without any real need, and with really bad consequences to the reader. Is adding "in terms of being referenced by a macro instead of a local variable" to the commit message that hard? When writing commit messages, you need to try to put yourself in the shoes of the reader. Aleksandar > >> @@ -4503,6 +4504,7 @@ static target_ulong disas_insn(DisasContext *s, > > CPUState *cpu) > >> s->rex_x = 0; > >> s->rex_b = 0; > >> s->rex_r = 0; > >> + s->rex_w = -1; > >> s->x86_64_hregs = false; > >> #endif > >> s->rip_offset = 0; /* for relative ip address */ > >> @@ -4514,7 +4516,6 @@ static target_ulong disas_insn(DisasContext *s, > > CPUState *cpu) > >> } > >> > >> prefixes = 0; > >> - rex_w = -1; > > > r~
On 8/15/19 6:19 AM, Aleksandar Markovic wrote: > > 15.08.2019. 11.55, "Richard Henderson" <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> је написао/ла: >> >> On 8/15/19 8:30 AM, Aleksandar Markovic wrote: >> > >> > 15.08.2019. 04.13, "Jan Bobek" <jan.bobek@gmail.com <mailto:jan.bobek@gmail.com> >> > <mailto:jan.bobek@gmail.com <mailto:jan.bobek@gmail.com>>> је написао/ла: >> >> >> >> From: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net> <mailto:rth@twiddle.net <mailto:rth@twiddle.net>>> >> >> >> >> Treat this the same as we already do for other rex bits. >> >> >> >> Signed-off-by: Richard Henderson <rth@twiddle.net <mailto:rth@twiddle.net> <mailto:rth@twiddle.net <mailto:rth@twiddle.net>>> >> >> --- >> >> target/i386/translate.c | 19 +++++++++++-------- >> >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/target/i386/translate.c b/target/i386/translate.c >> >> index d74dbfd585..c0866c2797 100644 >> >> --- a/target/i386/translate.c >> >> +++ b/target/i386/translate.c >> >> @@ -44,11 +44,13 @@ >> >> #define REX_X(s) ((s)->rex_x) >> >> #define REX_B(s) ((s)->rex_b) >> >> #define REX_R(s) ((s)->rex_r) >> >> +#define REX_W(s) ((s)->rex_w) >> >> #else >> >> #define CODE64(s) 0 >> >> #define REX_X(s) 0 >> >> #define REX_B(s) 0 >> >> #define REX_R(s) 0 >> >> +#define REX_W(s) -1 >> > >> > The commit message says "treat rex_w the same as other rex bits". Why is then >> > REX_W() treated differently here? >> >> "Treated the same" in terms of being referenced by a macro instead of a local >> variable. As for the -1, if you look at the rest of the patch you can clearly >> see it preserves existing behaviour. >> > > That is exactly what I dislike about your commit messages: they often introduce ambiguity, without any real need, and with really bad consequences to the reader. Is adding "in terms of being referenced by a macro instead of a local > variable" to the commit message that hard? > > When writing commit messages, you need to try to put yourself in the shoes of the reader. FWIW, personally I don't find it confusing. I think even just the first couple of lines of the patch make it quite clear what's going on. Just my 2 cents. -Jan
diff --git a/target/i386/translate.c b/target/i386/translate.c index d74dbfd585..c0866c2797 100644 --- a/target/i386/translate.c +++ b/target/i386/translate.c @@ -44,11 +44,13 @@ #define REX_X(s) ((s)->rex_x) #define REX_B(s) ((s)->rex_b) #define REX_R(s) ((s)->rex_r) +#define REX_W(s) ((s)->rex_w) #else #define CODE64(s) 0 #define REX_X(s) 0 #define REX_B(s) 0 #define REX_R(s) 0 +#define REX_W(s) -1 #endif #ifdef TARGET_X86_64 @@ -100,7 +102,7 @@ typedef struct DisasContext { #ifdef TARGET_X86_64 int lma; /* long mode active */ int code64; /* 64 bit code segment */ - int rex_x, rex_b, rex_r; + int rex_x, rex_b, rex_r, rex_w; #endif int vex_l; /* vex vector length */ int vex_v; /* vex vvvv register, without 1's complement. */ @@ -4495,7 +4497,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) int modrm, reg, rm, mod, op, opreg, val; target_ulong next_eip, tval; target_ulong pc_start = s->base.pc_next; - int rex_w; s->pc_start = s->pc = pc_start; s->override = -1; @@ -4503,6 +4504,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) s->rex_x = 0; s->rex_b = 0; s->rex_r = 0; + s->rex_w = -1; s->x86_64_hregs = false; #endif s->rip_offset = 0; /* for relative ip address */ @@ -4514,7 +4516,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) } prefixes = 0; - rex_w = -1; next_byte: b = x86_ldub_code(env, s); @@ -4557,7 +4558,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) case 0x40 ... 0x4f: if (CODE64(s)) { /* REX prefix */ - rex_w = (b >> 3) & 1; + s->rex_w = (b >> 3) & 1; s->rex_r = (b & 0x4) << 1; s->rex_x = (b & 0x2) << 2; s->rex_b = (b & 0x1) << 3; @@ -4606,7 +4607,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) s->rex_b = (~vex2 >> 2) & 8; #endif vex3 = x86_ldub_code(env, s); - rex_w = (vex3 >> 7) & 1; +#ifdef TARGET_X86_64 + s->rex_w = (vex3 >> 7) & 1; +#endif switch (vex2 & 0x1f) { case 0x01: /* Implied 0f leading opcode bytes. */ b = x86_ldub_code(env, s) | 0x100; @@ -4631,9 +4634,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) /* Post-process prefixes. */ if (CODE64(s)) { /* In 64-bit mode, the default data size is 32-bit. Select 64-bit - data with rex_w, and 16-bit data with 0x66; rex_w takes precedence + data with REX_W, and 16-bit data with 0x66; REX_W takes precedence over 0x66 if both are present. */ - dflag = (rex_w > 0 ? MO_64 : prefixes & PREFIX_DATA ? MO_16 : MO_32); + dflag = (REX_W(s) > 0 ? MO_64 : prefixes & PREFIX_DATA ? MO_16 : MO_32); /* In 64-bit mode, 0x67 selects 32-bit addressing. */ aflag = (prefixes & PREFIX_ADR ? MO_32 : MO_64); } else { @@ -5029,7 +5032,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) /* operand size for jumps is 64 bit */ ot = MO_64; } else if (op == 3 || op == 5) { - ot = dflag != MO_16 ? MO_32 + (rex_w == 1) : MO_16; + ot = dflag != MO_16 ? MO_32 + (REX_W(s) == 1) : MO_16; } else if (op == 6) { /* default push size is 64 bit */ ot = mo_pushpop(s, dflag);