Bug 162371 – [JSC] Use an inline cache to generate op_negate

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Use an inline cache to generate op_negate

[Benjamin Poulain]

Created attachment 289511 [details]
Patch

[Saam Barati]

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]

Created attachment 289630 [details]
Patch

[Saam Barati]

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]

Created attachment 289647 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289647 [details]
Patch

Clearing flags on attachment: 289647

Committed r206289: <http://trac.webkit.org/changeset/206289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.