Bug 68750 – Value profiling in baseline JIT for JSVALUE32_64

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yuqiang Xian]

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.

[Yuqiang Xian]

Created attachment 108578 [details]
proposed patch

[Filip Pizlo]

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?

[Yuqiang Xian]

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.

[Filip Pizlo]

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.

[Yuqiang Xian]

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.

[Geoffrey Garen]

Comment on attachment 108580 [details]
updated patch

Marking r- based on Phil's comments.

[Yuqiang Xian]

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.

[Gavin Barraclough]

Created attachment 109034 [details]
Yuqiang's patch, + profiling for op_div/op_mul

[Geoffrey Garen]

Comment on attachment 109034 [details]
Yuqiang's patch, + profiling for op_div/op_mul

r=me

[Gavin Barraclough]

Fixed in r96238