Bug 162371

Summary: [JSC] Use an inline cache to generate op_negate
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 162486    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Benjamin Poulain
Reported 2016-09-21 20:16:56 PDT
[JSC] Use an inline cache to generate op_negate
Attachments
Patch (57.71 KB, patch)
2016-09-21 20:22 PDT, Benjamin Poulain
no flags
Patch (60.34 KB, patch)
2016-09-22 16:40 PDT, Benjamin Poulain
no flags
Patch (64.34 KB, patch)
2016-09-22 19:04 PDT, Benjamin Poulain
no flags
Benjamin Poulain
Comment 1 2016-09-21 20:22:07 PDT
Saam Barati
Comment 2 2016-09-22 13:40:19 PDT
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*
Benjamin Poulain
Comment 3 2016-09-22 16:40:29 PDT
Saam Barati
Comment 4 2016-09-22 18:04:17 PDT
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?
Benjamin Poulain
Comment 5 2016-09-22 19:04:01 PDT
WebKit Commit Bot
Comment 6 2016-09-22 20:52:38 PDT
Comment on attachment 289647 [details] Patch Clearing flags on attachment: 289647 Committed r206289: <http://trac.webkit.org/changeset/206289>
WebKit Commit Bot
Comment 7 2016-09-22 20:52:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.