| Message ID | 1475230780-8669-1-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 30.09.2016 12:19, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Fix the decoding of iss_sf in disas_ld_lit. > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome) > is a bit that specifies the width of the register that the > instruction loads to. > > If cleared it specifies 32 bits. > If set it specifies 64 bits. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/translate-a64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index ddf52f5..eae25c3 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn) > do_fp_ld(s, rt, tcg_addr, size); > } else { > /* Only unsigned 32bit loads target 32bit registers. */ > - bool iss_sf = opc == 0 ? 32 : 64; > + bool iss_sf = opc == 0 ? false : true; You could simplify that to: bool iss_sf = !(opc == 0); Thomas
On Fri, Sep 30, 2016 at 12:42:27PM +0200, Thomas Huth wrote: > On 30.09.2016 12:19, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Fix the decoding of iss_sf in disas_ld_lit. > > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome) > > is a bit that specifies the width of the register that the > > instruction loads to. > > > > If cleared it specifies 32 bits. > > If set it specifies 64 bits. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/translate-a64.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > > index ddf52f5..eae25c3 100644 > > --- a/target-arm/translate-a64.c > > +++ b/target-arm/translate-a64.c > > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn) > > do_fp_ld(s, rt, tcg_addr, size); > > } else { > > /* Only unsigned 32bit loads target 32bit registers. */ > > - bool iss_sf = opc == 0 ? 32 : 64; > > + bool iss_sf = opc == 0 ? false : true; > > You could simplify that to: > > bool iss_sf = !(opc == 0); I don't really see how that is simpler/clearer. I considered: bool iss_sf = opc != 0; but felt the expanded one was clearer in particular as a patch for review. In any case, I have no strong opinion on the syntax. Cheers, Edgar
On 30 September 2016 at 03:49, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Sep 30, 2016 at 12:42:27PM +0200, Thomas Huth wrote: >> On 30.09.2016 12:19, Edgar E. Iglesias wrote: >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> > >> > Fix the decoding of iss_sf in disas_ld_lit. >> > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome) >> > is a bit that specifies the width of the register that the >> > instruction loads to. >> > >> > If cleared it specifies 32 bits. >> > If set it specifies 64 bits. >> > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> > --- >> > target-arm/translate-a64.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c >> > index ddf52f5..eae25c3 100644 >> > --- a/target-arm/translate-a64.c >> > +++ b/target-arm/translate-a64.c >> > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn) >> > do_fp_ld(s, rt, tcg_addr, size); >> > } else { >> > /* Only unsigned 32bit loads target 32bit registers. */ >> > - bool iss_sf = opc == 0 ? 32 : 64; >> > + bool iss_sf = opc == 0 ? false : true; >> >> You could simplify that to: >> >> bool iss_sf = !(opc == 0); > > I don't really see how that is simpler/clearer. > > I considered: > bool iss_sf = opc != 0; > > but felt the expanded one was clearer in particular as a patch for review. I would probably personally have written it without the ?:, but I don't think it's significant enough to bother respinning. thanks -- PMM
On 09/30/2016 12:34 PM, Peter Maydell wrote: >>>> + bool iss_sf = opc == 0 ? false : true; Feels like a double negative. >>> >>> You could simplify that to: >>> >>> bool iss_sf = !(opc == 0); >> >> I don't really see how that is simpler/clearer. >> >> I considered: >> bool iss_sf = opc != 0; Or even shorter: bool iss_sf = !opc;
Peter, feel free to change the syntax to whatever you feel is best. Cheers, Edgar --- Sent from my phone -------- Original Message -------- Subject: Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit From: Peter Maydell <peter.maydell@linaro.org> Date: 30 Sep 2016, 19:34 To: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> On 30 September 2016 at 03:49, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Sep 30, 2016 at 12:42:27PM +0200, Thomas Huth wrote: >> On 30.09.2016 12:19, Edgar E. Iglesias wrote: >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> > >> > Fix the decoding of iss_sf in disas_ld_lit. >> > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome) >> > is a bit that specifies the width of the register that the >> > instruction loads to. >> > >> > If cleared it specifies 32 bits. >> > If set it specifies 64 bits. >> > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> > --- >> > target-arm/translate-a64.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c >> > index ddf52f5..eae25c3 100644 >> > --- a/target-arm/translate-a64.c >> > +++ b/target-arm/translate-a64.c >> > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn) >> > do_fp_ld(s, rt, tcg_addr, size); >> > } else { >> > /* Only unsigned 32bit loads target 32bit registers. */ >> > - bool iss_sf = opc == 0 ? 32 : 64; >> > + bool iss_sf = opc == 0 ? false : true; >> >> You could simplify that to: >> >> bool iss_sf = !(opc == 0); > > I don't really see how that is simpler/clearer. > > I considered: > bool iss_sf = opc != 0; > > but felt the expanded one was clearer in particular as a patch for review. I would probably personally have written it without the ?:, but I don't think it's significant enough to bother respinning. thanks -- PMM This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
On 30 September 2016 at 14:42, Edgar Iglesias <edgar.iglesias@xilinx.com> wrote:
> Peter, feel free to change the syntax to whatever you feel is best.
Applied to target-arm.next, thanks. I picked "iss_sf = opc != 0;"
-- PMM
Thanks :-) Edgar --- Sent from my phone -------- Original Message -------- Subject: Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit From: Peter Maydell <peter.maydell@linaro.org> Date: 1 Oct 2016, 01:20 To: Edgar Iglesias <edgari@xilinx.com> On 30 September 2016 at 14:42, Edgar Iglesias <edgar.iglesias@xilinx.com> wrote: > Peter, feel free to change the syntax to whatever you feel is best. Applied to target-arm.next, thanks. I picked "iss_sf = opc != 0;" -- PMM This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c index ddf52f5..eae25c3 100644 --- a/target-arm/translate-a64.c +++ b/target-arm/translate-a64.c @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn) do_fp_ld(s, rt, tcg_addr, size); } else { /* Only unsigned 32bit loads target 32bit registers. */ - bool iss_sf = opc == 0 ? 32 : 64; + bool iss_sf = opc == 0 ? false : true; do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false, true, rt, iss_sf, false);