Valgrind currently lacks support for the new/enhanced instructions added to z/Architecture with the 13th edition from September 2019 (http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf). Not all new instructions need to be implemented, but only those expected to occur in programs compiled with -march=z15, in particular the new/enhanced instructions from the miscellaneous instruction extensions facility 3 and from the vector enhancements facility 2.
Created attachment 136581 [details] Support miscellaneous-instruction-extensions facility 3 This is the first part of arch13 (=z15) support. It adds support for the miscellaneous-instruction-extensions facility 3.
Created attachment 138815 [details] z15 support This updated patch includes support for both the miscellaneous-instructions-extensions facility 3 and the vector-enhancements facility 2.
Nice work! The following look OK to land as-is: Subject: [PATCH 01/13] s390x: Misc-insn-3, bitwise logical 3-way instructions Subject: [PATCH 02/13] s390x: Misc-insn-3, "select" instructions Subject: [PATCH 03/13] s390x: Misc-insn-3, new POPCNT variant Subject: [PATCH 04/13] s390x: Misc-insn-3, MVCRL Subject: [PATCH 06/13] s390x: Vec-enh-2, extend VSL, VSRA, and VSRL Subject: [PATCH 09/13] s390x: Vec-enh-2, VSLD and VSRD Subject: [PATCH 10/13] s390x: Vec-enh-2, VSTRS Subject: [PATCH 11/13] s390x: Mark arch13 features as supported Subject: [PATCH 12/13] s390x: Vec-enh-2, test cases Subject: [PATCH 13/13] s390x: Wrap up misc-insn-3 and vec-enh-2 support For patches 05/13 and 07/13, I have a couple of queries/comments, but nothing serious. Providing those assertions won't fail and that there's no build-time dependency problems for the test cases, OK to land these two too. ---------------------------------------------------------------- Subject: [PATCH 05/13] s390x: Misc-insn-3, test case none/tests/s390x/Makefile.am @@ -19,7 +19,8 @@ INSN_TESTS = clc clcle cvb cvd icm lpr tcxb lam_stam xc mvst add sub mul \ spechelper-ltr spechelper-or \ spechelper-icm-1 spechelper-icm-2 spechelper-tmll \ spechelper-tm laa vector lsc2 ppno vector_string vector_integer \ - vector_float add-z14 sub-z14 mul-z14 bic + vector_float add-z14 sub-z14 mul-z14 bic \ + misc3 Is `misc3` safe to always-build? Or would it require gating on assembler features, in the style of "if BUILD_DFP_TESTS .." just below? .. ah .. later .. I see misc3.c doesn't require any assembler support, since it just does `insn rrf,0x" #opcode "0000,%[out],%[a],%[b],0\n\t"`. diff --git a/none/tests/s390x/misc3.vgtest b/none/tests/s390x/misc3.vgtest new file mode 100644 index 000000000..d051a06bd --- /dev/null +++ b/none/tests/s390x/misc3.vgtest @@ -0,0 +1 @@ +prog: misc3 Similarly, does this require gating on host hwcaps? Looking at your _toIR.c implementation, maybe not, since that appears to be down-translating (I mean, translating from the z15 dialect into IR which will be re-emitted as instructions for some earlier zSeries CPU). I suppose this means that on older platforms, it will be possible to compile this test, and run it on V, but not to run it natively. ---------------------------------------------------------------- Subject: [PATCH 07/13] s390x: Vec-enh-2, extend VCDG, VCDLG, VCGD, and VCLGD + s390_insn_assert("vcdlg", m3 == 2 || m3 == 3); (multiple times) This will fail only on internal logic errors, correct? .. it can't fail only because the insn can't be decoded, right? Subject: [PATCH 08/13] s390x: Vec-enh-2, VLBR and friends +++ b/VEX/priv/host_s390_isel.c + case Iop_Perm8x16: Seeing use of Iop_Perm8x16 made me wonder if it had a definition that is independent of the guest/host endianness .. and, no it doesn't: libvex_ir.h says: for i in 0 .. 15 . result[i] = argL[ argR[i] ] so the meaning of "[n]" is itself ambiguous. Anyways, no need to change anything in your code.
(In reply to Julian Seward from comment #3) > Nice work! The following look OK to land as-is: > [...] Great, thanks for reviewing! > [...] > Is `misc3` safe to always-build? Or would it require gating on assembler > features, in the style of "if BUILD_DFP_TESTS .." just below? > > .. ah .. later .. I see misc3.c doesn't require any assembler support, > since it just does `insn rrf,0x" #opcode "0000,%[out],%[a],%[b],0\n\t"`. Right, I did consider using z15 mnemonics guarded by a configure switch, but it just unnecessarily complicated things. So I ended up with this backwards-compatible approach. > [...] > +prog: misc3 > > Similarly, does this require gating on host hwcaps? Looking at your > _toIR.c implementation, maybe not, since that appears to be > down-translating (I mean, translating from the z15 dialect into IR which > will be re-emitted as instructions for some earlier zSeries CPU). I > suppose this means that on older platforms, it will be possible to > compile this test, and run it on V, but not to run it natively. That's correct. I tested it on z14, and the test succeeded there, even though the executable crashes without Valgrind. > [...] > + s390_insn_assert("vcdlg", m3 == 2 || m3 == 3); > (multiple times) > This will fail only on internal logic errors, correct? .. it can't fail only > because the insn can't be decoded, right? Actually, the latter. The s390_insn_assert macro passes a special kind of decoding failure (a "specification exception") up through the call chain if the asserted condition is not met. > [...] > Seeing use of Iop_Perm8x16 made me wonder if it had a definition that > is independent of the guest/host endianness .. and, no it doesn't: > libvex_ir.h says: > > for i in 0 .. 15 . result[i] = argL[ argR[i] ] > > so the meaning of "[n]" is itself ambiguous. Anyways, no need to > change anything in your code. OK. I interpreted vec[n] to represent the vector's nth lane (starting with zero) when split up in 8-bit integer elements. And (at least on z/Architecture) this happens to be the same as the nth byte of the vector when stored in memory.
(In reply to Andreas Arnez from comment #4) > > + s390_insn_assert("vcdlg", m3 == 2 || m3 == 3); > > (multiple times) > > This will fail only on internal logic errors, correct? .. it can't fail only > > because the insn can't be decoded, right? > Actually, the latter. The s390_insn_assert macro passes a special kind of > decoding failure (a "specification exception") up through the call chain > if the asserted condition is not met. So .. am a bit unclear about this, but it doesn't matter. It's enough to say that it should be impossible to cause the front end to assert by feeding it invalid instructions. Instead, all invalid insns should cause a SIGILL to be handed to the guest (or whatever is appropriate on s390). So long as the above holds, then I'm happy.
(In reply to Julian Seward from comment #5) > So .. am a bit unclear about this, but it doesn't matter. It's enough to > say that it should be impossible to cause the front end to assert by feeding > it invalid instructions. Instead, all invalid insns should cause a SIGILL > to be handed to the guest (or whatever is appropriate on s390). So long > as the above holds, then I'm happy. Right, this type of decode failure results in a SIGILL to be synthesized, just like with other decode failures. So I think this addresses your concern. I'll apply the patch series then.
It seems a bit confusing to me that a function named foo_assert() doesn't do an assert()...
(In reply to Peter Maydell from comment #7) > It seems a bit confusing to me that a function named foo_assert() doesn't do > an assert()... Maybe. I'm open for suggestions for a better name. But since the macro hasn't been introduced by this patch series (it's just used here), I would defer that to a separate patch then.
OK, since there weren't any more comments, I pushed the series upstream, fixing this bug. Regarding the s390_insn_assert macro, please let me know if anyone has a suggestion for a better name.