Bug 68750 - Value profiling in baseline JIT for JSVALUE32_64
Summary: Value profiling in baseline JIT for JSVALUE32_64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-24 00:05 PDT by Yuqiang Xian
Modified: 2011-09-28 11:04 PDT (History)
2 users (show)

See Also:


Attachments
proposed patch (6.74 KB, patch)
2011-09-24 00:10 PDT, Yuqiang Xian
no flags Details | Formatted Diff | Diff
updated patch (10.14 KB, patch)
2011-09-24 00:41 PDT, Yuqiang Xian
ggaren: review-
Details | Formatted Diff | Diff
Yuqiang's patch, + profiling for op_div/op_mul (15.75 KB, patch)
2011-09-28 10:25 PDT, Gavin Barraclough
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuqiang Xian 2011-09-24 00:05:28 PDT
The work of enabling JSVALUE32_64 support in DFG JIT is ongoing. We need to add value profiling in baseline JIT for JSVALUE32_64 format.
Comment 1 Yuqiang Xian 2011-09-24 00:10:00 PDT
Created attachment 108578 [details]
proposed patch
Comment 2 Filip Pizlo 2011-09-24 00:13:10 PDT
Comment on attachment 108578 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108578&action=review

> Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:1072
>      unsigned op2 = currentInstruction[3].u.operand;
>      OperandTypes types = OperandTypes::fromInt(currentInstruction[4].u.operand);
>  
> +#if ENABLE(VALUE_PROFILER)
> +    m_codeBlock->addSpecialFastCaseProfile(m_bytecodeOffset);
> +#endif
> +
>      if (!supportsFloatingPoint()) {
>          addSlowCase(jump());
>          return;

Do you do the same thing for op_mul, to catch negative zero?

> Source/JavaScriptCore/jit/JITStubCall.h:214
> -        JIT::Call callWithValueProfiling(unsigned dst, JIT::ValueProfilingSiteKind)
> +        JIT::Call callWithValueProfiling(unsigned dst, JIT::ValueProfilingSiteKind kind)
>          {
> -            return call(dst);
> +            ASSERT(m_returnType == Value || m_returnType == Cell);
> +            JIT::Call call = this->call();
> +            m_jit->emitValueProfilingSite(kind);
> +            if (m_returnType == Value)
> +                m_jit->emitStore(dst, JIT::regT1, JIT::regT0);
> +            else
> +                m_jit->emitStoreCell(dst, JIT::returnValueRegister);
> +            return call;
>          }

Do you call this from anywhere in the 32_64 case?
Comment 3 Yuqiang Xian 2011-09-24 00:41:19 PDT
Created attachment 108580 [details]
updated patch

sorry... I was just focusing on passing certain cases and forgot some other pieces which were not covered yet. Now the patch is updated. Thanks.
Comment 4 Filip Pizlo 2011-09-24 00:52:27 PDT
Comment on attachment 108580 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108580&action=review

> Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:998
>      unsigned op2 = currentInstruction[3].u.operand;
>      OperandTypes types = OperandTypes::fromInt(currentInstruction[4].u.operand);
>  
> +#if ENABLE(VALUE_PROFILER)
> +    m_codeBlock->addSpecialFastCaseProfile(m_bytecodeOffset);
> +#endif
> +
>      JumpList notInt32Op1;
>      JumpList notInt32Op2;
>  

The special fast case profile for op_mul should count the number of times that we went to the double slow path only because of negative zero.  You're counting this every time.  That'll throw the DFG off, since it'll think that every single execution of every multiply creates a negative zero result every time.

> Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:1076
>      unsigned op2 = currentInstruction[3].u.operand;
>      OperandTypes types = OperandTypes::fromInt(currentInstruction[4].u.operand);
>  
> +#if ENABLE(VALUE_PROFILER)
> +    m_codeBlock->addSpecialFastCaseProfile(m_bytecodeOffset);
> +#endif
> +
>      if (!supportsFloatingPoint()) {
>          addSlowCase(jump());
>          return;

The special fast case profile for op_div should count a specific case, not all cases.  In this case, it should be counting the number of times that the division created a result that was not an integer.  You're counting every execution.  This will cause the DFG to think that every execution of a division creates a double, which will cause performance problems in a number of benchmarks.

It would be good to get this right on the first go, because DFG mis-speculation bugs are truly not fun to debug.
Comment 5 Yuqiang Xian 2011-09-24 01:04:03 PDT
Thanks for the review, Filip. But I guess I'm not counting the special case at all for now. I add a profile there (with counter always zero) just in order to make the DFG parser happy to find a profile without assertions, though the counter is always zero. Am I wrong? Thanks.
Comment 6 Geoffrey Garen 2011-09-24 15:05:28 PDT
Comment on attachment 108580 [details]
updated patch

Marking r- based on Phil's comments.
Comment 7 Yuqiang Xian 2011-09-28 02:00:20 PDT
Hi Filip,

What do you think about my Comment #5? Basically we just want to add a profile there (for mul and div) because DFG JIT expects one, but currently we don't count the special case at all, which means there should be no mis-speculation. We'll add real special case counting later. 

Thanks.
Comment 8 Gavin Barraclough 2011-09-28 10:25:04 PDT
Created attachment 109034 [details]
Yuqiang's patch, + profiling for op_div/op_mul
Comment 9 Geoffrey Garen 2011-09-28 10:43:09 PDT
Comment on attachment 109034 [details]
Yuqiang's patch, + profiling for op_div/op_mul

r=me
Comment 10 Gavin Barraclough 2011-09-28 11:04:12 PDT
Fixed in r96238