https://bugs.kde.org/show_bug.cgi?id=383010
--- Comment #1 from Julian Seward <[email protected]> --- This is a quite good start. Here are some initial comments. This isn't a full review but should give you some initial feedback. ------------------------------ +// AVX-512 - use a real CPUid +// If XGETBV, XSAVE and XRSTOR fail, might need to update XSaveOpt +void amd64g_dirtyhelper_CPUID_avx512 ( VexGuestAMD64State* st ) { I see that you're doing this in order to get started quickly. But in general this doesn't work, because there's no fixed relationship between what the CPUID of the host claims is supported and what you actually support in your patch. In the end you'll need to return your own values from CPUID, like all the other CPUID helper function variants do. ---------------------------------- +static IRExpr* mkV512 ( UInt val ) +{ + return IRExpr_Const(IRConst_V512(val)); +} Assuming that IRConst_V512 contains 64 bits, one for each byte of the value, this is wrong. UInt is a 32 bit value. You need to use ULong, which is always 64 bits. ---------------------------------- @@ -2511,7 +2796,7 @@ static IRTemp disAMode_copy2tmp ( IRExpr* addr64 ) static IRTemp disAMode ( /*OUT*/Int* len, const VexAbiInfo* vbi, Prefix pfx, Long delta, - /*OUT*/HChar* buf, Int extra_bytes ) + /*OUT*/HChar* buf, Int extra_bytes ) Nit: please don't include whitespace changes ---------------------------------- @@ -17797,7 +18164,7 @@ static Long dis_AESx ( const VexAbiInfo* vbi, Prefix pfx, use of mkU64 rather than mkIRExpr_HWord implies the assumption that the host's word size is 64-bit. */ UInt gstOffD = ymmGuestRegOffset(rG); - UInt gstOffL = regNoL == 16 ? OFFB_YMM16 : ymmGuestRegOffset(regNoL); + UInt gstOffL = regNoL == 32 ? OFFB_ZMM32 : ymmGuestRegOffset(regNoL); Did you mean to use zmmGuestRegOffset here? (I don't know if that even exists, but there's a Y/Z inconsistency here, when comparing to the OFFB_YMM16/ZMM32 text. ---------------------------------- + default: + // All other AVX-512 instructions. Crash before things get worse. + vex_printf("dis_ESC_0F__EVEX - UNRECOGNIZED OPCODE 0x%x\n", opcode); + vpanic(" bye\n "); + break; Please, don't cause the instruction decoder to panic on unimplemented instructions. It might be a while before you implement them all. Instead, use whatever the return conventions are for this function, to indicate decode failure (I think it is to return delta unchanged, but you should check) and let V's illegal-instruction handling deal with it in the normal way. The same applies for all the dis_ESC_* functions that you've added. ---------------------------------- Please use the house style as much as possible, in particular, 3-space indents. I notice you have 4 spaces in many places. + if (pfx & PFX_EVEX) + { + delta=ParseEVEX(&pfx, &esc, delta); + } House style please: { at the end of the condition. And spaces on each side of the =. ---------------------------------- + if (pfx & PFX_EVEX) { + /* EVEX prefixed instruction */ + Bool uses_vvvv = False; + switch (esc) { ... + default: + vassert(0); + } + Per comments above, please ensure we can't get to the vassert(0) unless the decoder is buggy. We should never get there for valid but unhandled instructions. ---------------------------------- diff --git a/VEX/priv/host_amd64_isel.c b/VEX/priv/host_amd64_isel.c + - vregmap2 and vregmap3 are used for 512-bit vector IRTemps. + Maybe it'd be better to use them for 256-bit, too Allocating these is going to be expensive for the 99.9% of blocks that don't require them. It would be nice to only allocate them if we know they will be required. ---------------------------------- + } +do_F64AssistedUnary: + { The use of gotos is OK (it makes the code simpler) but please indent them at the same level as surrounding code. Definitely not at column 1: + } + do_F64AssistedUnary: + { ---------------------------------- + for (int i=0; i<4; i++) { + dst[i] = newVRegV(env); + } (1) please, house style: spaces before/after = and < (2) where does '4' come from? Can you make it less like a magic number (give it a name?) +IRConst* IRConst_V512 ( UInt con ) +{ + IRConst* c = LibVEX_Alloc_inline(sizeof(IRConst)); + c->tag = Ico_V512; + c->Ico.V512 = con; + return c; +} Per comments above, this is definitely wrong if you intend to have one bit per byte for a 512 bit value. ---------------------------------- /* A union for doing 128-bit vector primitives conveniently. */ typedef union { @@ -78,6 +81,7 @@ typedef UShort w16[8]; UInt w32[4]; ULong w64[2]; + double f64[2]; use the house types: Double, not double. ---------------------------------- + UInt V512; /* 64-bit value; see Ico_V512 comment above */ per comments above ---------------------------------- + /* Conflict detection */ + Iop_Clz32x16, + Iop_CfD32x16, + Iop_BcMask_W2D, Please describe the semantics of these a bit. What do they do? ---------------------------------- case Ity_V256: tmp1 = assignNew('V', mce, Ity_I64, unop(Iop_1Sto64, tmp1)); - tmp1 = assignNew('V', mce, Ity_V128, binop(Iop_64HLtoV128, - tmp1, tmp1)); - tmp1 = assignNew('V', mce, Ity_V256, binop(Iop_V128HLtoV256, - tmp1, tmp1)); + tmp1 = assignNew('V', mce, Ity_V128, binop(Iop_64HLtoV128, tmp1, tmp1)); + tmp1 = assignNew('V', mce, Ity_V256, binop(Iop_V128HLtoV256, tmp1, tmp1)); + return tmp1; + case Ity_V512: + tmp1 = assignNew('V', mce, Ity_I64, unop(Iop_1Sto64, tmp1)); + tmp1 = assignNew('V', mce, Ity_V128, binop(Iop_64HLtoV128, tmp1, tmp1)); + tmp1 = assignNew('V', mce, Ity_V256, binop(Iop_V128HLtoV256, tmp1, tmp1)); + tmp1 = assignNew('V', mce, Ity_V512, binop(Iop_V256HLtoV512, tmp1, tmp1)); return tmp1; Why does this change the V256 case? This code is fragile and it would be better not to change handling of existing types. ---------------------------------- - if (UNLIKELY(ty == Ity_V256)) { + if (UNLIKELY((ty == Ity_V512))) { + Int offQ[8]; + IRDirty *diQ[8]; + IRAtom *addrQ[8], *vdataQ[8], *eBiasQ[8]; + if (end == Iend_LE) { + for (int i=0; i<8; i++) + offQ[i]=i*8; + } else { + for (int i=0; i<8; i++) + offQ[7-i]=i*8; + } + for (int i=0; i<8; i++){ + eBiasQ[i] = tyAddr==Ity_I32 ? mkU32(bias+offQ[i]) : mkU64(bias+offQ[i]); House style: - spaces around =, < - 3 char indent - proper indentation inside the loop - use house types (Int, not int) -- You are receiving this mail because: You are watching all bug changes.
