https://bugs.kde.org/show_bug.cgi?id=385408
--- Comment #19 from Julian Seward <jsew...@acm.org> ---
(In reply to Vadim Barkov from comment #18)
> Created attachment 108754 [details]
> Initial vector support (chapter 21) (fixed doc)
This is pretty good on the whole, but there are a couple of areas of concern.
Comments below.
+++ b/memcheck/mc_translate.c
+ case Iop_Perm8x16x2:
+ /* (V128, V128, V128) -> V128 */
+ return assignNew('V', mce, Ity_V128, triop(op, vatom1, vatom2,
vatom3));
This isn't right. It needs to steer the shadow values but using atom3 as
the steering value, not vatom3. Also it needs to somehow take into account
the fact that atom3 might be undefined. See the handling of Iop_Perm8x8 in
the same file.
VEX/priv/host_s390_defs.c
+s390_amode *
+s390_amode_for_stack_pointer(Int offset)
+{
+ if (fits_unsigned_12bit(offset))
+ return s390_amode_b12(offset, s390_hreg_stack_pointer());
+
+ if (fits_signed_20bit(offset))
+ return s390_amode_b20(offset, s390_hreg_stack_pointer());
If in both cases you're returning a 's390_amode*', is there any purpose
in having both variants? Why not always just use the 20 bit variant?
+s390_isel_vec_expr_wrk(ISelEnv *env, IRExpr *expr)
..
+ /* --------- LOAD --------- */
+ case Iex_Load: {
Nit: please indent the 'case' and the comment the same amount, here and below.
VEX/pub/libvex_guest_s390x.h
+ /* 64 */ union{ ULong guest_f0; V128 guest_v0; };
..
+ /* 304 */ union{ ULong guest_f15; V128 guest_v15; };
This is definitely not right, because the two union components have
different sizes (8 vs 16 bytes). Please remove the _f[0-15] variants and
the unions themselves, and instead use (eg) guest_vN.w64[0] or .w64[1]
instead. This is what various other ports do, that have similar register
arrangements.
VEX/priv/guest_s390_defs.h
+ S390_CC_VEC_LAST = 3 // supposed to be tha last element in enum
Typo: s/tha/the
VEX/priv/guest_s390_helpers.c
+/*------------------------------------------------------------*/
+/*--- Dirty helper for vector instructions ---*/
+/*------------------------------------------------------------*/
+#if defined(VGA_s390x)
Nit: please, ensure there is at least one blank line before/after these big
comment blocks, here and below
VEX/priv/guest_s390_toIR.c
+s390_format_VRX_VRRD(const HChar *(*irgen)(UChar v1, IRTemp op2addr),
+ UChar v1, UChar x2, UChar b2, UShort d2, UChar rxb)
+{
+ const HChar *mnm;
+ IRTemp op2addr = newTemp(Ity_I64);
+
+ if (! s390_host_has_vx) {
+ emulation_failure(EmFail_S390X_vx);
+ return;
+ }
Is this really right? Looking at emulation_failure(), I see that this
produces a jump of the kind Ijk_EmFail. For other targets, and
instruction-decode failure produces a jump of the kind Ijk_NoDecode, which
is different -- it causes Valgrind to send SIGILL to the guest.
So I don't think this is really right. Unless the s390 response to an
undecodeable insn is something different than SIGILL.
+s390_irgen_VMRH(UChar v1, UChar v2, UChar v3, UChar m4)
+s390_irgen_VMRL(UChar v1, UChar v2, UChar v3, UChar m4)
+s390_irgen_VUPH(UChar v1, UChar v2, UChar m3)
+s390_irgen_VUPLH(UChar v1, UChar v2, UChar m3)
+s390_irgen_VUPL(UChar v1, UChar v2, UChar m3)
+s390_irgen_VUPLL(UChar v1, UChar v2, UChar m3)
Many of these look like they are performing vector operations by doing one
lane at a time. Whilst this will generate working code, it will also cause
the translations to be big. In extreme cases of this (on other
architectures), given long sequences of vector instructions in the input, we
have had problems in which VEX's internal storage overflowed and so it had
to abort. I would therefore encourage you to generate shorter sequences
that make more extensive use of the vector IROps, if possible.
one/tests/s390x/vector.c
These tests test decoding, but they don't test the arithmetic much at all.
Please add more tests, possibly by running each test (eg) 50 times with
random data. That often shakes out edge cases. See for example
none/tests/amd64/avx-1.c.
--
You are receiving this mail because:
You are watching all bug changes.
