diff mbox

[02/21] target-arm: A64: add support for ld/st unsigned imm

Message ID 1387293144-11554-3-git-send-email-peter.maydell@linaro.org
State New
Headers show

Commit Message

Peter Maydell Dec. 17, 2013, 3:12 p.m. UTC 
From: Alex Bennée <alex@bennee.com>

This adds support for the forms of ld/st with a 12 bit
unsigned immediate offset.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate-a64.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

Comments

Richard Henderson Dec. 19, 2013, 5:46 p.m. UTC | #1 
On 12/17/2013 07:12 AM, Peter Maydell wrote:
> +        if (size == 3 && opc == 2) {
> +            /* PRFM - prefetch */
> +            return;
> +        }
> +        if (opc == 3 && size > 1) {
> +            unallocated_encoding(s);
> +            return;
> +        }
> +        is_store = (opc == 0);
> +        is_signed = opc & (1<<1);
> +        is_extended = (size < 3) && (opc & 1);

I thought we'd discussed rearranging this bit of decoding to better match the
ARM?  In particular, opc = 2 && size = 2 should be unallocated.

And please no (1<<1).


r~
Peter Maydell Dec. 20, 2013, 4:08 p.m. UTC | #2 
On 19 December 2013 17:46, Richard Henderson <rth@twiddle.net> wrote:
> On 12/17/2013 07:12 AM, Peter Maydell wrote:
>> +        if (size == 3 && opc == 2) {
>> +            /* PRFM - prefetch */
>> +            return;
>> +        }
>> +        if (opc == 3 && size > 1) {
>> +            unallocated_encoding(s);
>> +            return;
>> +        }
>> +        is_store = (opc == 0);
>> +        is_signed = opc & (1<<1);
>> +        is_extended = (size < 3) && (opc & 1);
>
> I thought we'd discussed rearranging this bit of decoding to better match the
> ARM?

Can't find anything in my email archive but I wouldn't be surprised
if I was just searching on the wrong keywords...

> In particular, opc = 2 && size = 2 should be unallocated.

This is LDRSW (immediate), not unallocated, isn't it?

I agree the decode logic isn't laid out the same as the ARM ARM,
but I'm pretty sure it's correct.

> And please no (1<<1).

Agreed.

-- PMM
Richard Henderson Dec. 20, 2013, 4:26 p.m. UTC | #3 
On 12/20/2013 08:08 AM, Peter Maydell wrote:
>> In particular, opc = 2 && size = 2 should be unallocated.
> 
> This is LDRSW (immediate), not unallocated, isn't it?
> 
> I agree the decode logic isn't laid out the same as the ARM ARM,
> but I'm pretty sure it's correct.

Oops, typo: opc=3 && size=2.  Basically,


  if opc<1> == '0' then
    // store or zero-extending load
    memop = if opc<0> == '1' then MemOp_LOAD else MemOp_STORE;
    regsize = if size == '11' then 64 else 32;
    signed = FALSE;
  else
    if size == '11' then
        memop = MemOp_PREFETCH;
        if opc<0> == '1' then UnallocatedEncoding();
    else
        // sign-extending load
        memop = MemOp_LOAD;
        if size == '10' && opc<0> == '1' then UnallocatedEncoding();

this one  ----^


r~
Peter Maydell Dec. 20, 2013, 4:29 p.m. UTC | #4 
On 20 December 2013 16:26, Richard Henderson <rth@twiddle.net> wrote:
> On 12/20/2013 08:08 AM, Peter Maydell wrote:
>>> In particular, opc = 2 && size = 2 should be unallocated.
>>
>> This is LDRSW (immediate), not unallocated, isn't it?
>>
>> I agree the decode logic isn't laid out the same as the ARM ARM,
>> but I'm pretty sure it's correct.
>
> Oops, typo: opc=3 && size=2.  Basically,
>
>
>   if opc<1> == '0' then
>     // store or zero-extending load
>     memop = if opc<0> == '1' then MemOp_LOAD else MemOp_STORE;
>     regsize = if size == '11' then 64 else 32;
>     signed = FALSE;
>   else
>     if size == '11' then
>         memop = MemOp_PREFETCH;
>         if opc<0> == '1' then UnallocatedEncoding();
>     else
>         // sign-extending load
>         memop = MemOp_LOAD;
>         if size == '10' && opc<0> == '1' then UnallocatedEncoding();
>
> this one  ----^

+        if (size == 3 && opc == 2) {
+            /* PRFM - prefetch */
+            return;
+        }
+        if (opc == 3 && size > 1) {
+            unallocated_encoding(s);
+            return;
+        }
+        is_store = (opc == 0);
+        is_signed = opc & (1<<1);
+        is_extended = (size < 3) && (opc & 1);

That is caught by 'if (opc == 3 && size > 1)'.

thanks
-- PMM
Richard Henderson Dec. 20, 2013, 4:44 p.m. UTC | #5 
On 12/20/2013 08:29 AM, Peter Maydell wrote:
> On 20 December 2013 16:26, Richard Henderson <rth@twiddle.net> wrote:
>> On 12/20/2013 08:08 AM, Peter Maydell wrote:
>>>> In particular, opc = 2 && size = 2 should be unallocated.
>>>
>>> This is LDRSW (immediate), not unallocated, isn't it?
>>>
>>> I agree the decode logic isn't laid out the same as the ARM ARM,
>>> but I'm pretty sure it's correct.
>>
>> Oops, typo: opc=3 && size=2.  Basically,
>>
>>
>>   if opc<1> == '0' then
>>     // store or zero-extending load
>>     memop = if opc<0> == '1' then MemOp_LOAD else MemOp_STORE;
>>     regsize = if size == '11' then 64 else 32;
>>     signed = FALSE;
>>   else
>>     if size == '11' then
>>         memop = MemOp_PREFETCH;
>>         if opc<0> == '1' then UnallocatedEncoding();
>>     else
>>         // sign-extending load
>>         memop = MemOp_LOAD;
>>         if size == '10' && opc<0> == '1' then UnallocatedEncoding();
>>
>> this one  ----^
> 
> +        if (size == 3 && opc == 2) {
> +            /* PRFM - prefetch */
> +            return;
> +        }
> +        if (opc == 3 && size > 1) {
> +            unallocated_encoding(s);
> +            return;
> +        }
> +        is_store = (opc == 0);
> +        is_signed = opc & (1<<1);
> +        is_extended = (size < 3) && (opc & 1);
> 
> That is caught by 'if (opc == 3 && size > 1)'.

Ah, right.  In which case, patches 2, 3, 4 get

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Peter Maydell Dec. 20, 2013, 4:52 p.m. UTC | #6 
On 20 December 2013 16:44, Richard Henderson <rth@twiddle.net> wrote:
> On 12/20/2013 08:29 AM, Peter Maydell wrote:
>> On 20 December 2013 16:26, Richard Henderson <rth@twiddle.net> wrote:
>>> On 12/20/2013 08:08 AM, Peter Maydell wrote:
>>>>> In particular, opc = 2 && size = 2 should be unallocated.
>>>>
>>>> This is LDRSW (immediate), not unallocated, isn't it?
>>>>
>>>> I agree the decode logic isn't laid out the same as the ARM ARM,
>>>> but I'm pretty sure it's correct.
>>>
>>> Oops, typo: opc=3 && size=2.  Basically,
>>>
>>>
>>>   if opc<1> == '0' then
>>>     // store or zero-extending load
>>>     memop = if opc<0> == '1' then MemOp_LOAD else MemOp_STORE;
>>>     regsize = if size == '11' then 64 else 32;
>>>     signed = FALSE;
>>>   else
>>>     if size == '11' then
>>>         memop = MemOp_PREFETCH;
>>>         if opc<0> == '1' then UnallocatedEncoding();
>>>     else
>>>         // sign-extending load
>>>         memop = MemOp_LOAD;
>>>         if size == '10' && opc<0> == '1' then UnallocatedEncoding();
>>>
>>> this one  ----^
>>
>> +        if (size == 3 && opc == 2) {
>> +            /* PRFM - prefetch */
>> +            return;
>> +        }
>> +        if (opc == 3 && size > 1) {
>> +            unallocated_encoding(s);
>> +            return;
>> +        }
>> +        is_store = (opc == 0);
>> +        is_signed = opc & (1<<1);
>> +        is_extended = (size < 3) && (opc & 1);
>>
>> That is caught by 'if (opc == 3 && size > 1)'.
>
> Ah, right.  In which case, patches 2, 3, 4 get
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks. I've fixed up the 1<<1 &c, so we now read

+    if (is_vector) {
+        size |= (opc & 2) << 1;
+        if (size > 4) {
+            unallocated_encoding(s);
+            return;
+        }
+        is_store = !extract32(opc, 0, 1);
+    } else {
+        if (size == 3 && opc == 2) {
+            /* PRFM - prefetch */
+            return;
+        }
+        if (opc == 3 && size > 1) {
+            unallocated_encoding(s);
+            return;
+        }
+        is_store = (opc == 0);
+        is_signed = extract32(opc, 1, 1);
+        is_extended = (size < 3) && extract32(opc, 0, 1);
+    }

Were you planning to review patches 12 ("Remove
ARMCPU/CPUARMState from cpregs APIs used by decoder")
and 19 ("Widen exclusive-access support struct fields
to 64 bits") ? I think those are the only two patches in
this set which I don't either have review comments to fix
or a reviewed-by from you now.

thanks
-- PMM
Richard Henderson Dec. 20, 2013, 4:57 p.m. UTC | #7 
On 12/20/2013 08:52 AM, Peter Maydell wrote:
> Were you planning to review patches 12 ("Remove
> ARMCPU/CPUARMState from cpregs APIs used by decoder")
> and 19 ("Widen exclusive-access support struct fields
> to 64 bits") ? I think those are the only two patches in
> this set which I don't either have review comments to fix
> or a reviewed-by from you now.

I skipped over those because they're pure target-arm internals, and not really
translator related.  But I could have a look later today if you like.


r~
Peter Maydell Dec. 20, 2013, 5:16 p.m. UTC | #8 
On 20 December 2013 16:57, Richard Henderson <rth@twiddle.net> wrote:
> On 12/20/2013 08:52 AM, Peter Maydell wrote:
>> Were you planning to review patches 12 ("Remove
>> ARMCPU/CPUARMState from cpregs APIs used by decoder")
>> and 19 ("Widen exclusive-access support struct fields
>> to 64 bits") ? I think those are the only two patches in
>> this set which I don't either have review comments to fix
>> or a reviewed-by from you now.
>
> I skipped over those because they're pure target-arm internals, and not really
> translator related.  But I could have a look later today if you like.

The 'widen exclusive access fields' one could use some eyeballs
to check I didn't mess up when I reworked the generated code from
looking at two 32 bit fields to one 64 bit field. You're right that the
other one is mostly target-arm internals though.

Mostly I just wanted to know if I should be waiting for more
review before sending out a respin :-)

thanks
-- PMM
diff mbox

Patch

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index e4ce038..3712a6d 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -899,10 +899,97 @@  static void disas_ldst_pair(DisasContext *s, uint32_t insn)
     }
 }
 
+/*
+ * C3.3.13 Load/store (unsigned immediate)
+ *
+ * 31 30 29   27  26 25 24 23 22 21        10 9     5
+ * +----+-------+---+-----+-----+------------+-------+------+
+ * |size| 1 1 1 | V | 0 1 | opc |   imm12    |  Rn   |  Rt  |
+ * +----+-------+---+-----+-----+------------+-------+------+
+ *
+ * For non-vector:
+ *   size: 00-> byte, 01 -> 16 bit, 10 -> 32bit, 11 -> 64bit
+ *   opc: 00 -> store, 01 -> loadu, 10 -> loads 64, 11 -> loads 32
+ * For vector:
+ *   size is opc<1>:size<1:0> so 100 -> 128 bit; 110 and 111 unallocated
+ *   opc<0>: 0 -> store, 1 -> load
+ * Rn: base address register (inc SP)
+ * Rt: target register
+ */
+static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn)
+{
+    int rt = extract32(insn, 0, 5);
+    int rn = extract32(insn, 5, 5);
+    unsigned int imm12 = extract32(insn, 10, 12);
+    bool is_vector = extract32(insn, 26, 1);
+    int size = extract32(insn, 30, 2);
+    int opc = extract32(insn, 22, 2);
+    unsigned int offset;
+
+    TCGv_i64 tcg_addr;
+
+    bool is_store;
+    bool is_signed = false;
+    bool is_extended = false;
+
+    if (is_vector) {
+        size |= (opc & 2) << 1;
+        if (size > 4) {
+            unallocated_encoding(s);
+            return;
+        }
+        is_store = ((opc & 1) == 0);
+    } else {
+        if (size == 3 && opc == 2) {
+            /* PRFM - prefetch */
+            return;
+        }
+        if (opc == 3 && size > 1) {
+            unallocated_encoding(s);
+            return;
+        }
+        is_store = (opc == 0);
+        is_signed = opc & (1<<1);
+        is_extended = (size < 3) && (opc & 1);
+    }
+
+    if (rn == 31) {
+        gen_check_sp_alignment(s);
+    }
+    tcg_addr = read_cpu_reg_sp(s, rn, 1);
+    offset = imm12 << size;
+    tcg_gen_addi_i64(tcg_addr, tcg_addr, offset);
+
+    if (is_vector) {
+        if (is_store) {
+            do_fp_st(s, rt, tcg_addr, size);
+        } else {
+            do_fp_ld(s, rt, tcg_addr, size);
+        }
+    } else {
+        TCGv_i64 tcg_rt = cpu_reg(s, rt);
+        if (is_store) {
+            do_gpr_st(s, tcg_rt, tcg_addr, size);
+        } else {
+            do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended);
+        }
+    }
+}
+
 /* Load/store register (all forms) */
 static void disas_ldst_reg(DisasContext *s, uint32_t insn)
 {
-    unsupported_encoding(s, insn);
+    switch (extract32(insn, 24, 2)) {
+    case 0:
+        unsupported_encoding(s, insn);
+        break;
+    case 1:
+        disas_ldst_reg_unsigned_imm(s, insn);
+        break;
+    default:
+        unallocated_encoding(s);
+        break;
+    }
 }
 
 /* AdvSIMD load/store multiple structures */