When loading from memory, load to our temporary vector first, so in case
we get an access exception on the second 64 bit element, the vector
won't get modified.

Loading with strange alingment from the end of the address space will
not properly wrap, we can ignore that for now.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/insn-data.def      |  3 +++
 target/s390x/translate_vx.inc.c | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index a3a0df7788..c6dd70f2fd 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -981,6 +981,9 @@
     F(0xe744, VGBM,    VRI_a, V,   0, 0, 0, 0, vgbm, 0, IF_VEC)
 /* VECTOR GENERATE MASK */
     F(0xe746, VGM,     VRI_b, V,   0, 0, 0, 0, vgm, 0, IF_VEC)
+/* VECTOR LOAD */
+    F(0xe706, VL,      VRX,   V,   la2, 0, 0, 0, vl, 0, IF_VEC)
+    F(0xe756, VLR,     VRR_a, V,   0, 0, 0, 0, vlr, 0, IF_VEC)
 
 #ifndef CONFIG_USER_ONLY
 /* COMPARE AND SWAP AND PURGE */
diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index ed63b2ca22..9af5639bfe 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -139,6 +139,9 @@ static void load_vec_element(DisasContext *s, uint8_t reg, uint8_t enr,
 
 #define gen_gvec_dup_i64(es, v1, c) \
     tcg_gen_gvec_dup_i64(es, vec_full_reg_offset(v1), 16, 16, c)
+#define gen_gvec_mov(v1, v2) \
+    tcg_gen_gvec_mov(0, vec_full_reg_offset(v1), vec_full_reg_offset(v2), 16, \
+                     16)
 
 static DisasJumpType op_vge(DisasContext *s, DisasOps *o)
 {
@@ -209,3 +212,18 @@ static DisasJumpType op_vgm(DisasContext *s, DisasOps *o)
     tcg_temp_free_i64(tmp);
     return DISAS_NEXT;
 }
+
+static DisasJumpType op_vl(DisasContext *s, DisasOps *o)
+{
+    load_vec_element(s, TMP_VREG_0, 0, o->addr1, MO_64);
+    gen_addi_and_wrap_i64(s, o->addr1, o->addr1, 8);
+    load_vec_element(s, TMP_VREG_0, 1, o->addr1, MO_64);
+    gen_gvec_mov(get_field(s->fields, v1), TMP_VREG_0);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_vlr(DisasContext *s, DisasOps *o)
+{
+    gen_gvec_mov(get_field(s->fields, v1), get_field(s->fields, v2));
+    return DISAS_NEXT;
+}
-- 
2.17.2

On 2/26/19 3:38 AM, David Hildenbrand wrote:
> +static DisasJumpType op_vl(DisasContext *s, DisasOps *o)
> +{
> +    load_vec_element(s, TMP_VREG_0, 0, o->addr1, MO_64);
> +    gen_addi_and_wrap_i64(s, o->addr1, o->addr1, 8);
> +    load_vec_element(s, TMP_VREG_0, 1, o->addr1, MO_64);
> +    gen_gvec_mov(get_field(s->fields, v1), TMP_VREG_0);
> +    return DISAS_NEXT;
> +}

Isn't it just as easy to load two TCGv_i64 temps and store into the correct
vector afterward?

Also, it is easy to honor the required alignment:

    TCGMemOp mop1, mop2;

    if (m3 < 3) {
        mop1 = mop2 = MO_TEQ;
    } else if (m3 == 3) {
        mop1 = mop2 = MO_TEQ | MO_ALIGN;
    } else {
        mop1 = MO_TEQ | MO_ALIGN_16;
        mop2 = MO_TEQ | MO_ALIGN;
    }
    tcg_gen_qemu_ld_i64(tmp1, o->addr1, mem_idx, mop1);
    gen_addi_and_wrap_i64(s, o->addr1, o->addr1, 8);
    tcg_gen_qemu_ld_i64(tmp2, o->addr1, mem_idx, mop2);
    write_vec_element_i64(tmp1, v1, 0, MO_64);
    write_vec_element_i64(tmp2, v1, 1, MO_64);


r~

On 27.02.19 16:39, Richard Henderson wrote:
> On 2/26/19 3:38 AM, David Hildenbrand wrote:
>> +static DisasJumpType op_vl(DisasContext *s, DisasOps *o)
>> +{
>> +    load_vec_element(s, TMP_VREG_0, 0, o->addr1, MO_64);
>> +    gen_addi_and_wrap_i64(s, o->addr1, o->addr1, 8);
>> +    load_vec_element(s, TMP_VREG_0, 1, o->addr1, MO_64);
>> +    gen_gvec_mov(get_field(s->fields, v1), TMP_VREG_0);
>> +    return DISAS_NEXT;
>> +}
> 
> Isn't it just as easy to load two TCGv_i64 temps and store into the correct
> vector afterward?

Yes it is, using the existing helpers was just easier. I guess I'll
change that.

> 
> Also, it is easy to honor the required alignment:

I think that would be wrong. It is only an alignment hint.

"Setting the alignment hint to a non-zero value
that doesn’t correspond to the alignment of the second operand may
reduce performance on some models."

So we must not inject an exception when unaligned. This, however would
be the result of MO_ALIGN,, right?

In essence, this is just an optimization for real hardware and can be
ignored by us completely.

> 
>     TCGMemOp mop1, mop2;
> 
>     if (m3 < 3) {
>         mop1 = mop2 = MO_TEQ;
>     } else if (m3 == 3) {
>         mop1 = mop2 = MO_TEQ | MO_ALIGN;
>     } else {
>         mop1 = MO_TEQ | MO_ALIGN_16;
>         mop2 = MO_TEQ | MO_ALIGN;
>     }
>     tcg_gen_qemu_ld_i64(tmp1, o->addr1, mem_idx, mop1);
>     gen_addi_and_wrap_i64(s, o->addr1, o->addr1, 8);
>     tcg_gen_qemu_ld_i64(tmp2, o->addr1, mem_idx, mop2);
>     write_vec_element_i64(tmp1, v1, 0, MO_64);
>     write_vec_element_i64(tmp2, v1, 1, MO_64);
> 
> 
> r~
> 


-- 

Thanks,

David / dhildenb

On 2/27/19 11:48 PM, David Hildenbrand wrote:
> I think that would be wrong. It is only an alignment hint.
> 
> "Setting the alignment hint to a non-zero value
> that doesn’t correspond to the alignment of the second operand may
> reduce performance on some models."
> 
> So we must not inject an exception when unaligned. This, however would
> be the result of MO_ALIGN,, right?

Ah, I didn't get that an alignment exception is not raised.  (I do find that
odd.  If the user is asserting a given alignment, why would we not tell him if
he is wrong?)

So, yes, ignore all of this from me -- leave MO_ALIGN off.


r~

On 28.02.19 17:34, Richard Henderson wrote:
> On 2/27/19 11:48 PM, David Hildenbrand wrote:
>> I think that would be wrong. It is only an alignment hint.
>>
>> "Setting the alignment hint to a non-zero value
>> that doesn’t correspond to the alignment of the second operand may
>> reduce performance on some models."
>>
>> So we must not inject an exception when unaligned. This, however would
>> be the result of MO_ALIGN,, right?
> 
> Ah, I didn't get that an alignment exception is not raised.  (I do find that
> odd.  If the user is asserting a given alignment, why would we not tell him if
> he is wrong?)

I was wondering the same thing. Most probably because they didn't
specify that that field has to contain 0 when introducing the
instruction. And as they added the alignment constraint only on new
hardware generations (z14), it could result for some instructions where
stuff "used to work" to suddenly report an exception.

> 
> So, yes, ignore all of this from me -- leave MO_ALIGN off.
> 
> 
> r~
> 


-- 

Thanks,

David / dhildenb