Bug 162371 - [JSC] Use an inline cache to generate op_negate
Summary: [JSC] Use an inline cache to generate op_negate
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
Depends on: 162486
  Show dependency treegraph
Reported: 2016-09-21 20:16 PDT by Benjamin Poulain
Modified: 2016-09-23 02:38 PDT (History)
5 users (show)

See Also:

Patch (57.71 KB, patch)
2016-09-21 20:22 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (60.34 KB, patch)
2016-09-22 16:40 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (64.34 KB, patch)
2016-09-22 19:04 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Saam Barati 2016-09-22 13:40:19 PDT
Comment on attachment 289511 [details]

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]
Comment 4 Saam Barati 2016-09-22 18:04:17 PDT
Comment on attachment 289630 [details]

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:

> 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);


> 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]
Comment 6 WebKit Commit Bot 2016-09-22 20:52:38 PDT
Comment on attachment 289647 [details]

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.