[Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar E. Iglesias]

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;
 
         do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false,
                   true, rt, iss_sf, false);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar E. Iglesias]

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

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

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;

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar Iglesias]

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.

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar Iglesias]

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.