Bug 162371

Created attachment 289511 [details] Patch

Comment on attachment 289511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289511&action=review Will continue to review later but just a couple comments > Source/JavaScriptCore/bytecode/ArithProfile.h:94 > + ASSERT(lhsObservedType().isEmpty()); Should be rhs not lhs > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3018 > +JITNegIC* CodeBlock::addJITNegIC() I believe you're missing code in CodeBlock also that switches on opcodes to determine if it has an ArithProfile*

Created attachment 289630 [details] Patch

Comment on attachment 289630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289630&action=review r=me w/ suggestions/comments. > Source/JavaScriptCore/bytecode/ArithProfile.h:95 > + ASSERT(lhsObservedType().isEmpty()); > + ASSERT(rhsObservedType().isEmpty()); Please ignore my comment here before, I'm it's ok just with lhsObservedType(). I think I commented because I thought you were asserting something else, but yeah, observed type obviously should start off as empty. I wonder if stylistically it's worth just coming up with a name instead of "lhs" for the single operand math ops. We can still reuse the 'lhs' slot, but maybe we can call it different things in different functions. Up to you. For example, it's a bit weird that the function below is simply called 'observeLHS' instead of something like 'observeInput' or something. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3018 > +JITNegIC* CodeBlock::addJITNegIC() I think you need to modify the implementations of: arithProfileForBytecodeOffset and arithProfileForPC > Source/JavaScriptCore/jit/JITArithmetic.cpp:832 > + mathICGenerationState.slowPathCall = callOperation(reinterpret_cast<J_JITOperation_EJMic>(repatchFunction), resultRegs, srcRegs, TrustedImmPtr(mathIC)); I know this is how I implemented it originally, but I wonder if it's worth also checking shouldSlowPathRepatch here. I guess after the first call of repatching, we will never try to repatch again, anyways. Maybe it's not worth changing. > Source/JavaScriptCore/jit/JITMathIC.h:76 > + if (!profileValidator(arithProfile)) { Style: The name validator confuses me here. Maybe we can call this "isProfileEmpty"? > Source/JavaScriptCore/jit/JITNegGenerator.cpp:67 > + jit.move(CCallHelpers::TrustedImm64((int64_t)(1ull << 63)), m_result.payloadGPR()); Style: I think this should be a static_cast by WebKit style. > Source/JavaScriptCore/jit/JITNegGenerator.cpp:70 > + jit.move(CCallHelpers::TrustedImm64((int64_t)(1ull << 63)), m_scratchGPR); ditto > Source/JavaScriptCore/jit/JITNegGenerator.cpp:75 > + jit.xor32(CCallHelpers::TrustedImm32(1 << 31), m_result.tagGPR()); Shouldn't this be the payload not tag?

Created attachment 289647 [details] Patch

Comment on attachment 289647 [details] Patch Clearing flags on attachment: 289647 Committed r206289: <http://trac.webkit.org/changeset/206289>

All reviewed patches have been landed. Closing bug.