| Message ID | 1265385151-11024-5-git-send-email-riku.voipio@iki.fi |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote: > From: Juha Riihimäki <juha.riihimaki@nokia.com> > > add an extra check in "two registers and a shift" to ensure element > size decoding logic cannot fail. > > Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com> > Signed-off-by: Riku Voipio <riku.voipio@nokia.com> > --- > target-arm/translate.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 743b846..8bba034 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -4567,8 +4567,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) > size = 3; > } else { > size = 2; > - while (size && (insn & (1 << (size + 19))) == 0) > + while (size && (insn & (1 << (size + 19))) == 0) { > size--; > + } > } > shift = (insn >> 16) & ((1 << (3 + size)) - 1); > /* To avoid excessive dumplication of ops we implement shift I think there's a patch ordering problem that makes the comment and the change not agree :-) Laurent
On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote: >> From: Juha Riihimäki <juha.riihimaki@nokia.com> >> >> add an extra check in "two registers and a shift" to ensure element >> size decoding logic cannot fail. >> >> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com> >> Signed-off-by: Riku Voipio <riku.voipio@nokia.com> >> --- >> target-arm/translate.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/target-arm/translate.c b/target-arm/translate.c >> index 743b846..8bba034 100644 >> --- a/target-arm/translate.c >> +++ b/target-arm/translate.c >> @@ -4567,8 +4567,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) >> size = 3; >> } else { >> size = 2; >> - while (size && (insn & (1 << (size + 19))) == 0) >> + while (size && (insn & (1 << (size + 19))) == 0) { >> size--; >> + } >> } >> shift = (insn >> 16) & ((1 << (3 + size)) - 1); >> /* To avoid excessive dumplication of ops we implement shift > > I think there's a patch ordering problem that makes > the comment and the change not agree :-) BTW I don't think adding the check for size is needed here. The encoding at that point looks like this: 3322222222221111111111 10987654321098765432109876543210 1111001_1___1______________1____ 1111001_1__1_______________1____ 1111001_1_1________________1____ so it will stop for size == 0 given that bit 19 will have to be set. Laurent
On Sun, Feb 07, 2010 at 02:02:31PM +0100, Laurent Desnogues wrote: > On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues > <laurent.desnogues@gmail.com> wrote: > > On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote: > >> From: Juha Riihimäki <juha.riihimaki@nokia.com> > >> add an extra check in "two registers and a shift" to ensure element > >> size decoding logic cannot fail. > > I think there's a patch ordering problem that makes > > the comment and the change not agree :-) Sorry, apparently messed up while rebasing. > BTW I don't think adding the check for size is needed > here. The encoding at that point looks like this: > 3322222222221111111111 > 10987654321098765432109876543210 > 1111001_1___1______________1____ > 1111001_1__1_______________1____ > 1111001_1_1________________1____ > so it will stop for size == 0 given that bit 19 will have to > be set. Juha agrees so we'll drop this patch (or more precisely get the actual change out of the previous patch..)
On Mon, Feb 8, 2010 at 12:47 PM, Riku Voipio <riku.voipio@iki.fi> wrote: > On Sun, Feb 07, 2010 at 02:02:31PM +0100, Laurent Desnogues wrote: >> On Sun, Feb 7, 2010 at 1:54 PM, Laurent Desnogues >> <laurent.desnogues@gmail.com> wrote: >> > On Fri, Feb 5, 2010 at 4:52 PM, Riku Voipio <riku.voipio@iki.fi> wrote: >> >> From: Juha Riihimäki <juha.riihimaki@nokia.com> > >> >> add an extra check in "two registers and a shift" to ensure element >> >> size decoding logic cannot fail. > >> > I think there's a patch ordering problem that makes >> > the comment and the change not agree :-) > > Sorry, apparently messed up while rebasing. > >> BTW I don't think adding the check for size is needed >> here. The encoding at that point looks like this: > >> 3322222222221111111111 >> 10987654321098765432109876543210 >> 1111001_1___1______________1____ >> 1111001_1__1_______________1____ >> 1111001_1_1________________1____ > >> so it will stop for size == 0 given that bit 19 will have to >> be set. > > Juha agrees so we'll drop this patch (or more precisely get the actual change > out of the previous patch..) Do you intend to resend the patch 3 of this set or should it be reviewed as is? Thanks, Laurent
diff --git a/target-arm/translate.c b/target-arm/translate.c index 743b846..8bba034 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -4567,8 +4567,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) size = 3; } else { size = 2; - while (size && (insn & (1 << (size + 19))) == 0) + while (size && (insn & (1 << (size + 19))) == 0) { size--; + } } shift = (insn >> 16) & ((1 << (3 + size)) - 1); /* To avoid excessive dumplication of ops we implement shift