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

Description Benjamin Poulain 2016-09-21 20:16:56 PDT
[JSC] Use an inline cache to generate op_negate
Comment 1 Benjamin Poulain 2016-09-21 20:22:07 PDT
Created attachment 289511 [details]
Patch
Comment 2 Saam Barati 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*
Comment 3 Benjamin Poulain 2016-09-22 16:40:29 PDT
Created attachment 289630 [details]
Patch
Comment 4 Saam Barati 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?
Comment 5 Benjamin Poulain 2016-09-22 19:04:01 PDT
Created attachment 289647 [details]
Patch
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2016-09-22 20:52:42 PDT
All reviewed patches have been landed.  Closing bug.