From 693dc1e1be7c306cd5cbc45ad012921837ad4d3c Mon Sep 17 00:00:00 2001 From: smith Date: Wed, 13 Mar 2013 08:32:40 -0700 Subject: [PATCH] Add new DestType enum for helpers that return TypedValue. This is necessary because without it, the only clue we have about what return type a helper has, is the register assignments of the destination SSATmp. It is possible that even though the helper returns a TypedValue, we might only have dest registers assigned for one part or the other part. Lastly, we could end up having helpers that return other two-register results, which shouldn't be confused with TypedValue. Also fixed cgFCallBuiltin() to use the correct size test instruction when computing on types. Types are currnetly 32bits and the other bits can be garbage. Renamed ArgGroup.valueType() to typedValue() for clarity. It's only for passing TypedValues by value; other register-packed structs would need new helpers. Added more asserts in cgCallHelper based on the return type. assert if we assigned a register but DestType implies there's no such value (e.g. assigning a void return to a register). --- hphp/runtime/vm/translator/hopt/codegen.cpp | 76 +++++++++++-------- hphp/runtime/vm/translator/hopt/codegen.h | 28 ++++--- .../vm/translator/hopt/nativecalls.cpp | 72 +++++++++--------- hphp/runtime/vm/translator/hopt/nativecalls.h | 6 +- 4 files changed, 102 insertions(+), 80 deletions(-) diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp index debcbdd46..9a5587371 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.cpp +++ b/hphp/runtime/vm/translator/hopt/codegen.cpp @@ -681,7 +681,7 @@ void CodeGenerator::cgCallNative(IRInstruction* inst) { argGroup.ssa(src); break; case TV: - argGroup.valueType(src); + argGroup.typedValue(src); break; case VecKeyS: argGroup.vectorKeyS(src); @@ -703,39 +703,44 @@ void CodeGenerator::cgCallNative(IRInstruction* inst) { } cgCallHelper(m_as, addr, - info.dest ? inst->getDst() : nullptr, + info.dest != DestType::None ? inst->getDst() : nullptr, info.sync, - argGroup); + argGroup, + info.dest); } void CodeGenerator::cgCallHelper(Asm& a, TCA addr, SSATmp* dst, SyncOptions sync, - ArgGroup& args) { + ArgGroup& args, + DestType destType) { PhysReg dstReg0 = InvalidReg; PhysReg dstReg1 = InvalidReg; if (dst) { dstReg0 = dst->getReg(0); dstReg1 = dst->getReg(1); } - return cgCallHelper(a, Transl::Call(addr), dstReg0, dstReg1, sync, args); + return cgCallHelper(a, Transl::Call(addr), dstReg0, dstReg1, sync, args, + destType); } void CodeGenerator::cgCallHelper(Asm& a, TCA addr, PhysReg dstReg, SyncOptions sync, - ArgGroup& args) { - cgCallHelper(a, Transl::Call(addr), dstReg, InvalidReg, sync, args); + ArgGroup& args, + DestType destType) { + cgCallHelper(a, Transl::Call(addr), dstReg, InvalidReg, sync, args, destType); } void CodeGenerator::cgCallHelper(Asm& a, const Transl::Call& call, PhysReg dstReg, SyncOptions sync, - ArgGroup& args) { - cgCallHelper(a, call, dstReg, InvalidReg, sync, args); + ArgGroup& args, + DestType destType) { + cgCallHelper(a, call, dstReg, InvalidReg, sync, args, destType); } void CodeGenerator::cgCallHelper(Asm& a, @@ -743,7 +748,8 @@ void CodeGenerator::cgCallHelper(Asm& a, PhysReg dstReg0, PhysReg dstReg1, SyncOptions sync, - ArgGroup& args) { + ArgGroup& args, + DestType destType) { assert(int(args.size()) <= kNumRegisterArgs); assert(m_curInst->isNative()); @@ -771,17 +777,20 @@ void CodeGenerator::cgCallHelper(Asm& a, recordSyncPoint(a, sync); } - // assume if dstReg1 is needed that the result is a TypedValue passed - // by value. In that case we need to rightshift the packed m_type - // enum to occupy the low 32bits of the dest register. - if (dstReg1 != InvalidReg) { - // dstReg1 contains m_type and m_aux, but we're expecting just the - // type in the lower 32 bits, so shift the 2nd result register. - a. shrq (kTypeShiftBits, reg::rdx); + // copy the call result to the destination register(s) + if (destType == DestType::TV) { + // rdx contains m_type and m_aux; we need to put just the type in the + // lower bits, so right-shift. rax contains m_data. + if (kTypeShiftBits > 0) a.shrq(kTypeShiftBits, reg::rdx); + shuffle2(a, reg::rax, reg::rdx, dstReg0, dstReg1); + } else if (destType == DestType::SSA) { + // copy the single-register result to dstReg0 + assert(dstReg1 == InvalidReg); + if (dstReg0 != InvalidReg) emitMovRegReg(a, reg::rax, dstReg0); + } else { + // void return type, no registers have values + assert(dstReg0 == InvalidReg && dstReg1 == InvalidReg); } - - // safely copy the return value (rax:rdx) to (dstReg0:dstReg1) - shuffle2(a, reg::rax, reg::rdx, dstReg0, dstReg1); } void CodeGenerator::cgMov(IRInstruction* inst) { @@ -1657,19 +1666,21 @@ void CodeGenerator::cgConv(IRInstruction* inst) { } else { Transl::Call helper(nullptr); ArgGroup args; - args.ssa(src); if (fromType == Type::Cell) { // Cell -> Bool - args.type(src); + args.typedValue(src); helper = Transl::Call((TCA)cellToBoolHelper); } else if (fromType.isString()) { // Str -> Bool + args.ssa(src); helper = Transl::Call(getMethodPtr(&StringData::toBoolean)); } else if (fromType.isArray()) { // Arr -> Bool + args.ssa(src); helper = Transl::Call((TCA)arrToBoolHelper); } else if (fromType == Type::Obj) { // Obj -> Bool + args.ssa(src); helper = Transl::Call(getMethodPtr(&ObjectData::o_toBoolean)); } else { // Dbl -> Bool @@ -1704,8 +1715,7 @@ void CodeGenerator::cgConv(IRInstruction* inst) { if (toType.isArray()) { ArgGroup args; - args.ssa(src); - args.type(src); + args.typedValue(src); ArrayData*(*fPtr)(TypedValue) = new_singleton_array_helper; cgCallHelper(m_as, (TCA)fPtr, dst, kNoSyncPoint, args); return; @@ -2030,7 +2040,7 @@ void CodeGenerator::cgLdSSwitchDestSlow(IRInstruction* inst) { inst->getDst(), kSyncPoint, ArgGroup() - .valueType(inst->getSrc(0)) + .typedValue(inst->getSrc(0)) .immPtr(strtab) .imm(data->numCases) .immPtr(jmptab)); @@ -3155,10 +3165,11 @@ void CodeGenerator::cgCallBuiltin(IRInstruction* inst) { if (returnType.subtypeOf(Type::Cell) || returnType.subtypeOf(Type::BoxedCell)) { m_as. loadl (returnBase[returnOffset + TVOFF(m_type)], r32(dstType)); - m_as. loadq (returnBase[returnOffset], dstReg); + m_as. loadq (returnBase[returnOffset + TVOFF(m_data)], dstReg); emitLoadImm(m_as, KindOfNull, rScratch); - static_assert(KindOfUninit == 0, "CallBuiltin needs update for KindOfUninit"); - m_as. testq (dstType, dstType); + static_assert(KindOfUninit == 0, + "CallBuiltin needs update for KindOfUninit"); + m_as. testl (r32(dstType), r32(dstType)); m_as. cmov_reg64_reg64 (CC_Z, rScratch, dstType); return; } @@ -4087,7 +4098,7 @@ void CodeGenerator::cgLookupClsCns(IRInstruction* inst) { .immPtr(cnsName->getValStr()); cgCallHelper(m_as, TCA(TargetCache::lookupClassConstantTv), - inst->getDst(), kSyncPoint, args); + inst->getDst(), kSyncPoint, args, DestType::TV); } HOT_FUNC_VM @@ -4315,7 +4326,8 @@ void CodeGenerator::cgReleaseVVOrExit(IRInstruction* inst) { TCA(static_cast(ExtraArgs::deallocate)), nullptr, kSyncPoint, - ArgGroup().reg(rFp) + ArgGroup().reg(rFp), + DestType::None ); }); } @@ -4386,7 +4398,7 @@ void CodeGenerator::cgConcat(IRInstruction* inst) { CG_PUNT(cgConcat); } cgCallHelper(m_as, (TCA)concat_value, dst, kNoSyncPoint, - ArgGroup().valueType(tl).valueType(tr)); + ArgGroup().typedValue(tl).typedValue(tr)); } } @@ -4461,7 +4473,7 @@ void CodeGenerator::cgDefFunc(IRInstruction* inst) { SSATmp* dst = inst->getDst(); SSATmp* func = inst->getSrc(0); cgCallHelper(m_as, (TCA)defFuncHelper, dst, kSyncPoint, - ArgGroup().ssa(func)); + ArgGroup().ssa(func), DestType::None); } void CodeGenerator::cgFillContThis(IRInstruction* inst) { diff --git a/hphp/runtime/vm/translator/hopt/codegen.h b/hphp/runtime/vm/translator/hopt/codegen.h index 7718d1090..bd683aec9 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.h +++ b/hphp/runtime/vm/translator/hopt/codegen.h @@ -38,6 +38,14 @@ class FailedCodeGen : public std::exception { struct ArgGroup; +// DestType describes the return type of native helper calls, particularly +// register assignments +enum class DestType : unsigned { + None, // return void (no valid registers) + SSA, // return a single-register value + TV // return a TypedValue packed in two registers +}; + enum SyncOptions { kNoSyncPoint, kSyncPoint, @@ -121,23 +129,27 @@ private: TCA addr, SSATmp* dst, SyncOptions sync, - ArgGroup& args); + ArgGroup& args, + DestType destType = DestType::SSA); void cgCallHelper(Asm& a, TCA addr, PhysReg dstReg, SyncOptions sync, - ArgGroup& args); + ArgGroup& args, + DestType destType = DestType::SSA); void cgCallHelper(Asm& a, const Transl::Call& call, PhysReg dstReg, SyncOptions sync, - ArgGroup& args); + ArgGroup& args, + DestType destType = DestType::SSA); void cgCallHelper(Asm& a, const Transl::Call& call, PhysReg dstReg0, PhysReg dstReg1, SyncOptions sync, - ArgGroup& args); + ArgGroup& args, + DestType destType = DestType::SSA); void cgStore(PhysReg base, int64_t off, @@ -416,7 +428,7 @@ struct ArgGroup { /* * Pass tmp as a TypedValue passed by value. */ - ArgGroup& valueType(SSATmp* tmp) { + ArgGroup& typedValue(SSATmp* tmp) { return ssa(tmp).type(tmp); } @@ -436,11 +448,9 @@ struct ArgGroup { private: ArgGroup& vectorKeyImpl(SSATmp* key, bool allowInt) { if (key->isString() || (allowInt && key->isA(Type::Int))) { - ssa(key).none(); - } else { - valueType(key); + return ssa(key).none(); } - return *this; + return typedValue(key); } std::vector m_args; diff --git a/hphp/runtime/vm/translator/hopt/nativecalls.cpp b/hphp/runtime/vm/translator/hopt/nativecalls.cpp index afa3dedd0..037ec61f9 100644 --- a/hphp/runtime/vm/translator/hopt/nativecalls.cpp +++ b/hphp/runtime/vm/translator/hopt/nativecalls.cpp @@ -40,8 +40,9 @@ static const SyncOptions SSyncAdj1 = kSyncPointAdjustOne; * (TCA) - Raw function pointer * {FSSA, idx} - Use a const TCA from inst->getSrc(idx) * Dest - * DSSA - The helper returns a value to be assigned to inst->getDst() - * DNone - The helper does not return a value + * DestType::SSA - The helper returns a single-register value + * DestType::TV - The helper returns a TypedValue in two registers + * DestType::None - The helper does not return a value * SyncPoint * SNone - The helper does not need a sync point * SSync - The helper needs a normal sync point @@ -57,63 +58,66 @@ static const SyncOptions SSyncAdj1 = kSyncPointAdjustOne; */ static CallMap s_callMap({ /* Opcode, Func, Dest, SyncPoint, Args */ - {AddElemStrKey, (TCA)addElemStringKeyHelper, DSSA, SNone, + {AddElemStrKey, (TCA)addElemStringKeyHelper, DestType::SSA, SNone, {{SSA, 0}, {SSA, 1}, {TV, 2}}}, - {AddElemIntKey, (TCA)addElemIntKeyHelper, DSSA, SNone, + {AddElemIntKey, (TCA)addElemIntKeyHelper, DestType::SSA, SNone, {{SSA, 0}, {SSA, 1}, {TV, 2}}}, - {AddNewElem, (TCA)&HphpArray::AddNewElemC, DSSA, SNone, + {AddNewElem, (TCA)&HphpArray::AddNewElemC, DestType::SSA, SNone, {{SSA, 0}, {TV, 1}}}, - {ArrayAdd, (TCA)array_add, DSSA, SNone, {{SSA, 0}, {SSA, 1}}}, - {Box, (TCA)box_value, DSSA, SNone, {{TV, 0}}}, - {LdObjMethod, (TCA)TargetCache::MethodCache::lookup, - DSSA, SSync, {{SSA, 0}, {SSA, 2}, {SSA, 1}}}, - {NewArray, (TCA)new_array, DSSA, SNone, {{SSA, 0}}}, - {NewTuple, (TCA)new_tuple, DSSA, SNone, + {ArrayAdd, (TCA)array_add, DestType::SSA, SNone, {{SSA, 0}, {SSA, 1}}}, - {PrintStr, (TCA)print_string, DNone, SNone, {{SSA, 0}}}, - {PrintInt, (TCA)print_int, DNone, SNone, {{SSA, 0}}}, - {PrintBool, (TCA)print_boolean, DNone, SNone, {{SSA, 0}}}, - {RaiseUninitLoc, (TCA)raiseUndefVariable, DNone, SSync, {{SSA, 0}}}, + {Box, (TCA)box_value, DestType::SSA, SNone, {{TV, 0}}}, + {LdObjMethod, (TCA)TargetCache::MethodCache::lookup, + DestType::SSA, SSync, + {{SSA, 0}, {SSA, 2}, {SSA, 1}}}, + {NewArray, (TCA)new_array, DestType::SSA, SNone, {{SSA, 0}}}, + {NewTuple, (TCA)new_tuple, DestType::SSA, SNone, + {{SSA, 0}, {SSA, 1}}}, + {PrintStr, (TCA)print_string, DestType::None, SNone, {{SSA, 0}}}, + {PrintInt, (TCA)print_int, DestType::None, SNone, {{SSA, 0}}}, + {PrintBool, (TCA)print_boolean, DestType::None, SNone, {{SSA, 0}}}, + {RaiseUninitLoc, (TCA)raiseUndefVariable, DestType::None, SSync, + {{SSA, 0}}}, /* Switch helpers */ - {LdSwitchDblIndex, (TCA)switchDoubleHelper, DSSA, SSync, + {LdSwitchDblIndex, (TCA)switchDoubleHelper, DestType::SSA, SSync, {{SSA, 0}, {SSA, 1}, {SSA, 2}}}, - {LdSwitchStrIndex, (TCA)switchStringHelper, DSSA, SSync, + {LdSwitchStrIndex, (TCA)switchStringHelper, DestType::SSA, SSync, {{SSA, 0}, {SSA, 1}, {SSA, 2}}}, - {LdSwitchObjIndex, (TCA)switchObjHelper, DSSA, SSync, + {LdSwitchObjIndex, (TCA)switchObjHelper, DestType::SSA, SSync, {{SSA, 0}, {SSA, 1}, {SSA, 2}}}, /* Continuation support helpers */ - {CreateCont, {FSSA, 0}, DSSA, SNone, + {CreateCont, {FSSA, 0}, DestType::SSA, SNone, {{SSA, 0}, {SSA, 1}, {SSA, 2}, {SSA, 3}, {SSA, 4}}}, - {FillContLocals, - (TCA)&VMExecutionContext::fillContinuationVars, - DNone, SNone, {{SSA, 0}, {SSA, 1}, {SSA, 2}, {SSA, 3}, {SSA, 4}}}, + {FillContLocals, (TCA)&VMExecutionContext::fillContinuationVars, + DestType::None, SNone, + {{SSA, 0}, {SSA, 1}, {SSA, 2}, {SSA, 3}, {SSA, 4}}}, /* VectorTranslator helpers */ - {BaseG, {FSSA, 0}, DSSA, SSync, {{TV, 1}, {SSA, 2}}}, - {PropX, {FSSA, 0}, DSSA, SSync, + {BaseG, {FSSA, 0}, DestType::SSA, SSync, {{TV, 1}, {SSA, 2}}}, + {PropX, {FSSA, 0}, DestType::SSA, SSync, {{SSA, 1}, {SSA, 2}, {VecKeyS, 3}, {SSA, 4}}}, - {CGetProp, {FSSA, 0}, DSSA, SSync, + {CGetProp, {FSSA, 0}, DestType::TV, SSync, {{SSA, 1}, {SSA, 2}, {VecKeyS, 3}, {SSA, 4}}}, - {SetProp, {FSSA, 0}, DSSA, SSync, + {SetProp, {FSSA, 0}, DestType::TV, SSync, {{SSA, 1}, {SSA, 2}, {VecKeyS, 3}, {TV, 4}}}, - {ElemX, {FSSA, 0}, DSSA, SSync, + {ElemX, {FSSA, 0}, DestType::SSA, SSync, {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}}, - {ElemDX, {FSSA, 0}, DSSA, SSync, + {ElemDX, {FSSA, 0}, DestType::SSA, SSync, {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}}, - {CGetElem, {FSSA, 0}, DSSA, SSync, + {CGetElem, {FSSA, 0}, DestType::TV, SSync, {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}}, - {SetElem, {FSSA, 0}, DSSA, SSync, + {SetElem, {FSSA, 0}, DestType::TV, SSync, {{SSA, 1}, {VecKeyIS, 2}, {TV, 3}}}, - {SetNewElem, (TCA)setNewElem, DSSA, SSync, {{SSA, 0}, {TV, 1}}}, - {IssetElem,{FSSA, 0}, DSSA, SSync, + {SetNewElem, (TCA)setNewElem, DestType::TV, SSync, {{SSA, 0}, {TV, 1}}}, + {IssetElem,{FSSA, 0}, DestType::SSA, SSync, {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}}, - {EmptyElem,{FSSA, 0}, DSSA, SSync, + {EmptyElem,{FSSA, 0}, DestType::SSA, SSync, {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}}, /* debug assert helpers */ - {DbgAssertPtr, (TCA)assertTv, DNone, SNone, {{SSA, 0}}}, + {DbgAssertPtr, (TCA)assertTv, DestType::None, SNone, {{SSA, 0}}}, }); CallMap::CallMap(CallInfoList infos) { diff --git a/hphp/runtime/vm/translator/hopt/nativecalls.h b/hphp/runtime/vm/translator/hopt/nativecalls.h index fcb9b376c..d47becf72 100644 --- a/hphp/runtime/vm/translator/hopt/nativecalls.h +++ b/hphp/runtime/vm/translator/hopt/nativecalls.h @@ -33,6 +33,7 @@ enum FuncType : unsigned { FPtr, FSSA, }; + struct FuncPtr { FuncPtr() {} FuncPtr(TCA f) : type(FPtr), ptr(f) {} @@ -45,11 +46,6 @@ struct FuncPtr { }; }; -enum DestType : unsigned { - DNone, - DSSA, -}; - enum ArgType : unsigned { SSA, TV,