| Message ID | 1396555000-8205-25-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 03.04.2014 21:56, Richard Henderson wrote: > The definition of op_type wasn't encoded for the proper shift for > the field, making the implementations confusing. > > Signed-off-by: Richard Henderson <rth@twiddle.net> At the end of the day the magic values remain in the load/store instructions though. Can we find a way to replace them with INSN_-something like for the others? I think I was doing something of the sort in a now obsolete patch I suggested some time early this year, see if it helps: http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05074.html Claudio > --- > tcg/aarch64/tcg-target.c | 42 +++++++++++++++++------------------------- > 1 file changed, 17 insertions(+), 25 deletions(-) > > diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c > index 9a2e4a6..a538a87 100644 > --- a/tcg/aarch64/tcg-target.c > +++ b/tcg/aarch64/tcg-target.c > @@ -242,12 +242,12 @@ static const enum aarch64_cond_code tcg_cond_to_aarch64[] = { > [TCG_COND_LEU] = COND_LS, > }; > > -enum aarch64_ldst_op_type { /* type of operation */ > - LDST_ST = 0x0, /* store */ > - LDST_LD = 0x4, /* load */ > - LDST_LD_S_X = 0x8, /* load and sign-extend into Xt */ > - LDST_LD_S_W = 0xc, /* load and sign-extend into Wt */ > -}; > +typedef enum { > + LDST_ST = 0, /* store */ > + LDST_LD = 1, /* load */ > + LDST_LD_S_X = 2, /* load and sign-extend into Xt */ > + LDST_LD_S_W = 3, /* load and sign-extend into Wt */ > +} AArch64LdstType; > > /* We encode the format of the insn into the beginning of the name, so that > we can have the preprocessor help "typecheck" the insn vs the output > @@ -483,22 +483,19 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, > } > > > -static inline void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, > - enum aarch64_ldst_op_type op_type, > - TCGReg rd, TCGReg rn, intptr_t offset) > +static void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, AArch64LdstType type, > + TCGReg rd, TCGReg rn, intptr_t offset) > { > /* use LDUR with BASE register with 9bit signed unscaled offset */ > - tcg_out32(s, 0x38000000 | size << 30 | op_type << 20 > + tcg_out32(s, 0x38000000 | size << 30 | type << 22 > | (offset & 0x1ff) << 12 | rn << 5 | rd); > } > > /* tcg_out_ldst_12 expects a scaled unsigned immediate offset */ > -static inline void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, > - enum aarch64_ldst_op_type op_type, > - TCGReg rd, TCGReg rn, > - tcg_target_ulong scaled_uimm) > +static void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, AArch64LdstType type, > + TCGReg rd, TCGReg rn, tcg_target_ulong scaled_uimm) > { > - tcg_out32(s, 0x39000000 | size << 30 | op_type << 20 > + tcg_out32(s, 0x39000000 | size << 30 | type << 22 > | scaled_uimm << 10 | rn << 5 | rd); > } > > @@ -637,21 +634,16 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, > } > } > > -static inline void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, > - enum aarch64_ldst_op_type op_type, > - TCGReg rd, TCGReg base, TCGReg regoff) > +static void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, AArch64LdstType type, > + TCGReg rd, TCGReg base, TCGReg regoff) > { > - /* load from memory to register using base + 64bit register offset */ > - /* using f.e. STR Wt, [Xn, Xm] 0xb8600800|(regoff << 16)|(base << 5)|rd */ > - /* the 0x6000 is for the "no extend field" */ > - tcg_out32(s, 0x38206800 | size << 30 | op_type << 20 > + tcg_out32(s, 0x38206800 | size << 30 | type << 22 > | regoff << 16 | base << 5 | rd); > } > > /* solve the whole ldst problem */ > -static inline void tcg_out_ldst(TCGContext *s, TCGMemOp size, > - enum aarch64_ldst_op_type type, > - TCGReg rd, TCGReg rn, intptr_t offset) > +static void tcg_out_ldst(TCGContext *s, TCGMemOp size, AArch64LdstType type, > + TCGReg rd, TCGReg rn, intptr_t offset) > { > if (offset >= -256 && offset < 256) { > tcg_out_ldst_9(s, size, type, rd, rn, offset); >
On 04/07/2014 04:45 AM, Claudio Fontana wrote: > On 03.04.2014 21:56, Richard Henderson wrote: >> The definition of op_type wasn't encoded for the proper shift for >> the field, making the implementations confusing. >> >> Signed-off-by: Richard Henderson <rth@twiddle.net> > > At the end of the day the magic values remain in the load/store instructions though. > Can we find a way to replace them with INSN_-something like for the others? > > I think I was doing something of the sort in a now obsolete patch I suggested some time early this year, see if it helps: > > http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05074.html Yes, we can. I'll do something for v3, > > Claudio > >> --- >> tcg/aarch64/tcg-target.c | 42 +++++++++++++++++------------------------- >> 1 file changed, 17 insertions(+), 25 deletions(-) >> >> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c >> index 9a2e4a6..a538a87 100644 >> --- a/tcg/aarch64/tcg-target.c >> +++ b/tcg/aarch64/tcg-target.c >> @@ -242,12 +242,12 @@ static const enum aarch64_cond_code tcg_cond_to_aarch64[] = { >> [TCG_COND_LEU] = COND_LS, >> }; >> >> -enum aarch64_ldst_op_type { /* type of operation */ >> - LDST_ST = 0x0, /* store */ >> - LDST_LD = 0x4, /* load */ >> - LDST_LD_S_X = 0x8, /* load and sign-extend into Xt */ >> - LDST_LD_S_W = 0xc, /* load and sign-extend into Wt */ >> -}; >> +typedef enum { >> + LDST_ST = 0, /* store */ >> + LDST_LD = 1, /* load */ >> + LDST_LD_S_X = 2, /* load and sign-extend into Xt */ >> + LDST_LD_S_W = 3, /* load and sign-extend into Wt */ >> +} AArch64LdstType; >> >> /* We encode the format of the insn into the beginning of the name, so that >> we can have the preprocessor help "typecheck" the insn vs the output >> @@ -483,22 +483,19 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, >> } >> >> >> -static inline void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, >> - enum aarch64_ldst_op_type op_type, >> - TCGReg rd, TCGReg rn, intptr_t offset) >> +static void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, AArch64LdstType type, >> + TCGReg rd, TCGReg rn, intptr_t offset) >> { >> /* use LDUR with BASE register with 9bit signed unscaled offset */ >> - tcg_out32(s, 0x38000000 | size << 30 | op_type << 20 >> + tcg_out32(s, 0x38000000 | size << 30 | type << 22 >> | (offset & 0x1ff) << 12 | rn << 5 | rd); >> } >> >> /* tcg_out_ldst_12 expects a scaled unsigned immediate offset */ >> -static inline void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, >> - enum aarch64_ldst_op_type op_type, >> - TCGReg rd, TCGReg rn, >> - tcg_target_ulong scaled_uimm) >> +static void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, AArch64LdstType type, >> + TCGReg rd, TCGReg rn, tcg_target_ulong scaled_uimm) >> { >> - tcg_out32(s, 0x39000000 | size << 30 | op_type << 20 >> + tcg_out32(s, 0x39000000 | size << 30 | type << 22 >> | scaled_uimm << 10 | rn << 5 | rd); >> } >> >> @@ -637,21 +634,16 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, >> } >> } >> >> -static inline void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, >> - enum aarch64_ldst_op_type op_type, >> - TCGReg rd, TCGReg base, TCGReg regoff) >> +static void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, AArch64LdstType type, >> + TCGReg rd, TCGReg base, TCGReg regoff) >> { >> - /* load from memory to register using base + 64bit register offset */ >> - /* using f.e. STR Wt, [Xn, Xm] 0xb8600800|(regoff << 16)|(base << 5)|rd */ >> - /* the 0x6000 is for the "no extend field" */ >> - tcg_out32(s, 0x38206800 | size << 30 | op_type << 20 >> + tcg_out32(s, 0x38206800 | size << 30 | type << 22 >> | regoff << 16 | base << 5 | rd); >> } >> >> /* solve the whole ldst problem */ >> -static inline void tcg_out_ldst(TCGContext *s, TCGMemOp size, >> - enum aarch64_ldst_op_type type, >> - TCGReg rd, TCGReg rn, intptr_t offset) >> +static void tcg_out_ldst(TCGContext *s, TCGMemOp size, AArch64LdstType type, >> + TCGReg rd, TCGReg rn, intptr_t offset) >> { >> if (offset >= -256 && offset < 256) { >> tcg_out_ldst_9(s, size, type, rd, rn, offset); >> >
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com> On 07.04.2014 16:31, Richard Henderson wrote: > On 04/07/2014 04:45 AM, Claudio Fontana wrote: >> On 03.04.2014 21:56, Richard Henderson wrote: >>> The definition of op_type wasn't encoded for the proper shift for >>> the field, making the implementations confusing. >>> >>> Signed-off-by: Richard Henderson <rth@twiddle.net> >> >> At the end of the day the magic values remain in the load/store instructions though. >> Can we find a way to replace them with INSN_-something like for the others? >> >> I think I was doing something of the sort in a now obsolete patch I suggested some time early this year, see if it helps: >> >> http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05074.html > > Yes, we can. I'll do something for v3, > >> >> Claudio >> >>> --- >>> tcg/aarch64/tcg-target.c | 42 +++++++++++++++++------------------------- >>> 1 file changed, 17 insertions(+), 25 deletions(-) >>> >>> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c >>> index 9a2e4a6..a538a87 100644 >>> --- a/tcg/aarch64/tcg-target.c >>> +++ b/tcg/aarch64/tcg-target.c >>> @@ -242,12 +242,12 @@ static const enum aarch64_cond_code tcg_cond_to_aarch64[] = { >>> [TCG_COND_LEU] = COND_LS, >>> }; >>> >>> -enum aarch64_ldst_op_type { /* type of operation */ >>> - LDST_ST = 0x0, /* store */ >>> - LDST_LD = 0x4, /* load */ >>> - LDST_LD_S_X = 0x8, /* load and sign-extend into Xt */ >>> - LDST_LD_S_W = 0xc, /* load and sign-extend into Wt */ >>> -}; >>> +typedef enum { >>> + LDST_ST = 0, /* store */ >>> + LDST_LD = 1, /* load */ >>> + LDST_LD_S_X = 2, /* load and sign-extend into Xt */ >>> + LDST_LD_S_W = 3, /* load and sign-extend into Wt */ >>> +} AArch64LdstType; >>> >>> /* We encode the format of the insn into the beginning of the name, so that >>> we can have the preprocessor help "typecheck" the insn vs the output >>> @@ -483,22 +483,19 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, >>> } >>> >>> >>> -static inline void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, >>> - enum aarch64_ldst_op_type op_type, >>> - TCGReg rd, TCGReg rn, intptr_t offset) >>> +static void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, AArch64LdstType type, >>> + TCGReg rd, TCGReg rn, intptr_t offset) >>> { >>> /* use LDUR with BASE register with 9bit signed unscaled offset */ >>> - tcg_out32(s, 0x38000000 | size << 30 | op_type << 20 >>> + tcg_out32(s, 0x38000000 | size << 30 | type << 22 >>> | (offset & 0x1ff) << 12 | rn << 5 | rd); >>> } >>> >>> /* tcg_out_ldst_12 expects a scaled unsigned immediate offset */ >>> -static inline void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, >>> - enum aarch64_ldst_op_type op_type, >>> - TCGReg rd, TCGReg rn, >>> - tcg_target_ulong scaled_uimm) >>> +static void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, AArch64LdstType type, >>> + TCGReg rd, TCGReg rn, tcg_target_ulong scaled_uimm) >>> { >>> - tcg_out32(s, 0x39000000 | size << 30 | op_type << 20 >>> + tcg_out32(s, 0x39000000 | size << 30 | type << 22 >>> | scaled_uimm << 10 | rn << 5 | rd); >>> } >>> >>> @@ -637,21 +634,16 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, >>> } >>> } >>> >>> -static inline void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, >>> - enum aarch64_ldst_op_type op_type, >>> - TCGReg rd, TCGReg base, TCGReg regoff) >>> +static void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, AArch64LdstType type, >>> + TCGReg rd, TCGReg base, TCGReg regoff) >>> { >>> - /* load from memory to register using base + 64bit register offset */ >>> - /* using f.e. STR Wt, [Xn, Xm] 0xb8600800|(regoff << 16)|(base << 5)|rd */ >>> - /* the 0x6000 is for the "no extend field" */ >>> - tcg_out32(s, 0x38206800 | size << 30 | op_type << 20 >>> + tcg_out32(s, 0x38206800 | size << 30 | type << 22 >>> | regoff << 16 | base << 5 | rd); >>> } >>> >>> /* solve the whole ldst problem */ >>> -static inline void tcg_out_ldst(TCGContext *s, TCGMemOp size, >>> - enum aarch64_ldst_op_type type, >>> - TCGReg rd, TCGReg rn, intptr_t offset) >>> +static void tcg_out_ldst(TCGContext *s, TCGMemOp size, AArch64LdstType type, >>> + TCGReg rd, TCGReg rn, intptr_t offset) >>> { >>> if (offset >= -256 && offset < 256) { >>> tcg_out_ldst_9(s, size, type, rd, rn, offset); >>> >> > >
diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 9a2e4a6..a538a87 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -242,12 +242,12 @@ static const enum aarch64_cond_code tcg_cond_to_aarch64[] = { [TCG_COND_LEU] = COND_LS, }; -enum aarch64_ldst_op_type { /* type of operation */ - LDST_ST = 0x0, /* store */ - LDST_LD = 0x4, /* load */ - LDST_LD_S_X = 0x8, /* load and sign-extend into Xt */ - LDST_LD_S_W = 0xc, /* load and sign-extend into Wt */ -}; +typedef enum { + LDST_ST = 0, /* store */ + LDST_LD = 1, /* load */ + LDST_LD_S_X = 2, /* load and sign-extend into Xt */ + LDST_LD_S_W = 3, /* load and sign-extend into Wt */ +} AArch64LdstType; /* We encode the format of the insn into the beginning of the name, so that we can have the preprocessor help "typecheck" the insn vs the output @@ -483,22 +483,19 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, } -static inline void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, - enum aarch64_ldst_op_type op_type, - TCGReg rd, TCGReg rn, intptr_t offset) +static void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, AArch64LdstType type, + TCGReg rd, TCGReg rn, intptr_t offset) { /* use LDUR with BASE register with 9bit signed unscaled offset */ - tcg_out32(s, 0x38000000 | size << 30 | op_type << 20 + tcg_out32(s, 0x38000000 | size << 30 | type << 22 | (offset & 0x1ff) << 12 | rn << 5 | rd); } /* tcg_out_ldst_12 expects a scaled unsigned immediate offset */ -static inline void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, - enum aarch64_ldst_op_type op_type, - TCGReg rd, TCGReg rn, - tcg_target_ulong scaled_uimm) +static void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, AArch64LdstType type, + TCGReg rd, TCGReg rn, tcg_target_ulong scaled_uimm) { - tcg_out32(s, 0x39000000 | size << 30 | op_type << 20 + tcg_out32(s, 0x39000000 | size << 30 | type << 22 | scaled_uimm << 10 | rn << 5 | rd); } @@ -637,21 +634,16 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, } } -static inline void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, - enum aarch64_ldst_op_type op_type, - TCGReg rd, TCGReg base, TCGReg regoff) +static void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, AArch64LdstType type, + TCGReg rd, TCGReg base, TCGReg regoff) { - /* load from memory to register using base + 64bit register offset */ - /* using f.e. STR Wt, [Xn, Xm] 0xb8600800|(regoff << 16)|(base << 5)|rd */ - /* the 0x6000 is for the "no extend field" */ - tcg_out32(s, 0x38206800 | size << 30 | op_type << 20 + tcg_out32(s, 0x38206800 | size << 30 | type << 22 | regoff << 16 | base << 5 | rd); } /* solve the whole ldst problem */ -static inline void tcg_out_ldst(TCGContext *s, TCGMemOp size, - enum aarch64_ldst_op_type type, - TCGReg rd, TCGReg rn, intptr_t offset) +static void tcg_out_ldst(TCGContext *s, TCGMemOp size, AArch64LdstType type, + TCGReg rd, TCGReg rn, intptr_t offset) { if (offset >= -256 && offset < 256) { tcg_out_ldst_9(s, size, type, rd, rn, offset);
The definition of op_type wasn't encoded for the proper shift for the field, making the implementations confusing. Signed-off-by: Richard Henderson <rth@twiddle.net> --- tcg/aarch64/tcg-target.c | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-)