Bug 404406

Created attachment 120110 [details]
Patches for z14 miscellaneous instruction support

These patches should provide the functionality requested in this Bug.

In summary, they address the following:
Patch #1: Adds the new machine models z14 and z14 ZR1.
Patch #2: Cleans up s390-check-opcodes.pl, to fix false positives when checking s390-opcodes.csv.
Patch #3: Implements the requested instructions.  Patch by Ilya Leoshkevich.
Patch #4: Adds tests for the new instructions.  Patch by Ilya Leoshkevich.

(In reply to Andreas Arnez from comment #1)

> Patch #1: Adds the new machine models z14 and z14 ZR1.

> Patch #2: Cleans up s390-check-opcodes.pl, to fix false positives when
> checking s390-opcodes.csv.

> Patch #4: Adds tests for the new instructions.  Patch by Ilya Leoshkevich.

These 3 look fine; OK to land.



> Patch #3: Implements the requested instructions.  Patch by Ilya Leoshkevich.

One minor and one maybe not so minor thing.  First the small thing:

--- a/VEX/priv/guest_s390_toIR.c
+++ b/VEX/priv/guest_s390_toIR.c

+static const HChar *
+s390_irgen_BIC(UChar r1, IRTemp op2addr)
+{
+   IRTemp cond = newTemp(Ity_I32);
+
+   if (r1 == 0) {
+   } else if (r1 == 15) {

I assume the empty { } for the case r1 == 0 is intended, yes?  If so
please put " /* nothing */" inside the { } so as to make this clear.


The maybe not so minor thing is: 

In host_s390_isel.c, is ISelEnv::hwcaps checked before creating a MG or MGRK
instruction?  If it isn't, and the host doesn't have that capability, but by
mistake a MG/MGRK instruction is created, then the system will fail with
SIGILL on the host (I assume).  If the check is in place then at least we'll
get an assertion from somewhere inside host_s390_isel.c, saying "I can't
reduce this tree" (select insns for it).

Created attachment 120343 [details]
Fixup that adds /* nothing */ and hwcaps check

(In reply to Ilya Leoshkevich from comment #3)
> Created attachment 120343 [details]
> Fixup that adds /* nothing */ and hwcaps check
Julian, I think this patch addresses your concern with accidentally generating an MG or MGRK instruction.  It also adds the comment, as requested.  Do you see anything else we may have missed?

(In reply to Andreas Arnez from comment #4)
> (In reply to Ilya Leoshkevich from comment #3)

Andreas, Ilya, thanks for the fixes.

regarding this:

--- a/VEX/priv/host_s390_isel.c
+++ b/VEX/priv/host_s390_isel.c
@@ -1018,6 +1018,7 @@ s390_isel_int128_expr_wrk(HReg *dst_hi, HReg *dst_lo, ISelEnv *env,
          goto do_multiply64;
 
       case Iop_MullS64:
+         vassert(env->hwcaps & VEX_HWCAPS_S390X_MI2);
          is_signed_multiply = True;
          goto do_multiply64;

Hmm, sorry. I should have been more precise.  Two changes instead of the above
assertion:

(1) At the end of s390_isel_int128_expr_wrk(), add a label "irreducible:" and two
lines following it.  Copy them from the same at the end of s390_isel_int_expr_wrk.

(2) Where your diff above has an assert, remove it and instead add
if (!(env->hwcaps & VEX_HWCAPS_S390X_MI2))) goto irreducible;

That's generally how the instruction selectors deal with trees that they can't reduce.

Created attachment 120373 [details]
updated fixup

Hi Julian, thanks for the explanation. I've added the label.

Since Ilya has changed the assertion as requested, are there any more comments?  
If nobody objects, I'd like to push this early next week.

Since there were no further comments, I pushed the following commits:

65d8e9ed9 s390x: Add models "z14" and "z14 ZR1"
e63f93a97 s390x: Clean up s390-check-opcodes.pl
50b20aa24 Bug 404406 - s390x: implement z14 miscellaneous instructions
91d53d116 Bug 404406 - s390x: test z14 miscellaneous instructions