https://bugs.kde.org/show_bug.cgi?id=429352
--- Comment #4 from Julian Seward <jsew...@acm.org> ---
(In reply to Carl Love from comment #0)
> Created attachment 133458 [details]
> Functional support for ISA 3.1, New Iops
>
> Power PC missing ISA 3.1 support. Additional patches, part 7
It seems to me that the mc_translate.c sections of the patch aren't right,
which could cause Memcheck to assert when running programs containing the new
instructions.
------
diff --git a/memcheck/mc_translate.c b/memcheck/mc_translate.c
@@ -3410,8 +3412,7 @@ IRAtom* expr2vbits_Triop ( MCEnv* mce,
unary64Fx2_w_rm(mce, vatom1, vatom2),
unary64Fx2_w_rm(mce, vatom1, vatom3)));
-
- default:
+ default:
Nit: return the `default:` to its correct indentation
------
+ // CARLL not sure about this yet.
Remove this pls
------
- do_And_Or:
+ /* Int 128-bit Integer two arg */
+ // CARLL not sure about this yet.
+ case Iop_DivU128:
+ case Iop_DivS128:
+ case Iop_DivU128E:
+ case Iop_DivS128E:
+ case Iop_ModU128:
+ case Iop_ModS128:
+ uifu = mkUifUV128; difd = mkDifDV128;
+ and_or_ty = Ity_V128; improve = mkImproveORV128; goto do_And_Or;
+
This strikes me as wrong. `do_And_Or` is only intended to instrument AND and
OR operations. But here, you're using it to instrument Div/Mod operations,
which isn't correct.
------
@@ -5001,15 +5024,18 @@ IRExpr* expr2vbits_Unop ( MCEnv* mce, IROp op, IRAtom*
atom )
return assignNew('V', mce, Ity_I64, unop(Iop_128HIto64, vatom));
case Iop_F128LOtoF64: /* F128 -> low half of F128 */
case Iop_D128LOtoD64: /* D128 -> low half of D128 */
+ case Iop_TruncF128toI128S: /* F128 -> I128S */
+ case Iop_TruncF128toI128U: /* F128 -> I128U */
return assignNew('V', mce, Ity_I64, unop(Iop_128to64, vatom));
This also doesn't seem correct to me, in the sense that it simply ignores the
definedness of the most significant 64 bits of the input, and, I am guessing,
will create general incorrectly-typed IR. Has it been tested? These cases
TruncF128toI128S and TruncF128toI128U do not belong with F128LOtoF64 and
D128LOtoD64. I suggest you add them instead to the group that includes
NegF128, AbsF128 and RndF128.
------
But even that (present code) doesn't seem correct:
case Iop_NegF128:
case Iop_AbsF128:
case Iop_RndF128:
case Iop_TruncF128toI64S: /* F128 -> I64S */
case Iop_TruncF128toI32S: /* F128 -> I32S (result stored in 64-bits) */
case Iop_TruncF128toI64U: /* F128 -> I64U */
case Iop_TruncF128toI32U: /* F128 -> I32U (result stored in 64-bits) */
return mkPCastTo(mce, Ity_I128, vatom);
NegF128/AbsF128/RndF128 are correct, because their shadow value will be an
I128 type and hence it is correct to do ` mkPCastTo(mce, Ity_I128, vatom)`.
But TruncF128toI64S and Iop_TruncF128toI64U need to be PCast-ed to an I64
value -- so they belong somewhere else in this file, not here, and
TruncF128toI32S and TruncF128toI32U need to be PCast-ed to an I32 value,
again, somewhere else.
Similarly these 6
case Iop_ReinterpF64asI64:
case Iop_ReinterpI64asF64:
case Iop_ReinterpI32asF32:
case Iop_ReinterpF32asI32:
case Iop_ReinterpI64asD64:
case Iop_ReinterpD64asI64:
are probably not correct; they need to be PCasted to I64/I32 appropriately.
------
Some smaller things, in other places in the patch:
diff --git a/VEX/priv/guest_ppc_toIR.c b/VEX/priv/guest_ppc_toIR.c
+static void generate_store_DFP_FPRF_value( ULong irType, IRExpr *src,
+ const VexAbiInfo* vbi )
...
+ } else {
+ vex_printf("ERROR generate_store_DFP_FPRF_value, unknown value for
irType 0x%lx\n", (unsigned long ) irType);
+ }
Uhh .. this isn't good. Please remove it, and either assert/panic here if
reaching here indicates a bug in this front end, or cause this to generate a
SIGILL (insn-not-decoded) in the normal way.
------
+ }
+ break;
}
Indent the `break` correctly, so it's easier to see what scope it's relevant
to.
------
diff --git a/VEX/priv/host_ppc_defs.c b/VEX/priv/host_ppc_defs.c
+PPCInstr* PPCInstr_AvTrinaryInt128 ( PPCAvOp op, HReg dst,
+ HReg src1, HReg src2, HReg src3 ) {
I believe the normal terminology for a 3-way thing is "ternary", not "trinary"
(I've never heard the latter word before, AFAIR)
------
@@ -4902,13 +5075,27 @@ Int emit_PPCInstr ( /*MB_MOD*/Bool* is_profInc,
case Pfp_IDUTOQ: // xscvudqp
p = mkFormVXR0( p, 63, fr_dst, 2, fr_src, 836, 0, endness_host );
break;
+ case Pfp_IQSTOQ: // xscvsqqp
+// vex_printf("CARLL, issue xscvsqqp instruction. If not on P10, I
will crash now on illegal inst.\n");
+ p = mkFormVXR0( p, 63, fr_dst, 11, fr_src, 836, 0, endness_host );
+ break;
+ case Pfp_IQUTOQ: // xscvuqqp
+// vex_printf("CARLL, issue xscvuqqp instruction. If not on P10, I
will crash now on illegal inst.\n");
+ p = mkFormVXR0( p, 63, fr_dst, 3, fr_src, 836, 0, endness_host );
+ break;
Remove the commented-out debug printing, pls
------
+// vex_printf("CARLL dctfixqq, if not running on P10, going to crash
on illegal inst.\n");
+// FAKE INST CARLL
+// p = mkFormVX( p, 4, dstVSR, 10, 10, 1156, endness_host ); //vor
+//REAL CALL
And here.
------
case Iop_I64StoF128: vex_printf("I64StoF128"); return;
+ case Iop_I128StoF128: vex_printf("1284StoF128"); return;
Typo, "1284"
--
You are receiving this mail because:
You are watching all bug changes.
