Bug 132470

I'll take this. I'm currently working on https://bugs.webkit.org/show_bug.cgi?id=151793 which needed to address this.

Created attachment 268194 [details] proposed patch. This patch has passed the layout tests and JSC tests. Benchmark results shows perf to be neutral in general with a 14% speed up on ftl-polymorphic-mul on x86_64 (Int52 is only available on 64-bit ports). Benchmark results to be uploaded shortly.

Created attachment 268195 [details] x86_64 benchmark result.

Created attachment 268197 [details] x86 benchmark result.

Comment on attachment 268194 [details] proposed patch. Investigating iOS build failure.

Comment on attachment 268194 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=268194&action=review Looks about right, but I think I found a bug. > Source/JavaScriptCore/bytecode/CodeBlock.h:1065 > + HashMap<uint32_t, uint32_t, IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_bytecodeOffsetToResultProfileIndexMap; Slight preference to say "unsigned" instead of "uint32_t". I think that's what we do elsehwere. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:372 > + static const int64_t int52OverflowPoint = (1ll << 51); > + int64_t int64Val = static_cast<int64_t>(std::abs(doubleVal)); > + if (int64Val >= int52OverflowPoint) Pretty sure this is wrong. The negative overflow point doesn't have the same absolute value as the positive overflow point. Why not use CheckedMath from WTF to do the check?

Comment on attachment 268194 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=268194&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.h:1065 >> + HashMap<uint32_t, uint32_t, IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_bytecodeOffsetToResultProfileIndexMap; > > Slight preference to say "unsigned" instead of "uint32_t". I think that's what we do elsehwere. Sure, I can do that. >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:372 >> + if (int64Val >= int52OverflowPoint) > > Pretty sure this is wrong. The negative overflow point doesn't have the same absolute value as the positive overflow point. Why not use CheckedMath from WTF to do the check? Yes, I'm aware that we might get a false positive on the negative overflow (1 out of 51 bits worth of numbers), but I considered that an OK tradeoff for simplicity. I was not aware of CheckedMath. Let me look into it.

(In reply to comment #7) > Comment on attachment 268194 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268194&action=review > > >> Source/JavaScriptCore/bytecode/CodeBlock.h:1065 > >> + HashMap<uint32_t, uint32_t, IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_bytecodeOffsetToResultProfileIndexMap; > > > > Slight preference to say "unsigned" instead of "uint32_t". I think that's what we do elsehwere. > > Sure, I can do that. > > >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:372 > >> + if (int64Val >= int52OverflowPoint) > > > > Pretty sure this is wrong. The negative overflow point doesn't have the same absolute value as the positive overflow point. Why not use CheckedMath from WTF to do the check? > > Yes, I'm aware that we might get a false positive on the negative overflow > (1 out of 51 bits worth of numbers), but I considered that an OK tradeoff > for simplicity. I was not aware of CheckedMath. Let me look into it. I see. I'm fine with your approach, if you add a comment that the imprecision is intentional. It may be that CheckedMath makes it easier to do it right. You'll have to do the int52 arithmetic using int64, so left-shift before checking.

(In reply to comment #8) > (In reply to comment #7) > > >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:372 > > >> + if (int64Val >= int52OverflowPoint) > > > > > > Pretty sure this is wrong. The negative overflow point doesn't have the same absolute value as the positive overflow point. Why not use CheckedMath from WTF to do the check? > > > > Yes, I'm aware that we might get a false positive on the negative overflow > > (1 out of 51 bits worth of numbers), but I considered that an OK tradeoff > > for simplicity. I was not aware of CheckedMath. Let me look into it. > > I see. I'm fine with your approach, if you add a comment that the > imprecision is intentional. > > It may be that CheckedMath makes it easier to do it right. You'll have to > do the int52 arithmetic using int64, so left-shift before checking. After looking into Checked<> math usage, I see that I would need to add the following to the slow path functions: 1. extract the JSValue numbers into an Int64 value. 2. Shift the Int64 values. 3. Convert them into Checked<> values. 4. Do the desired operation on the Checked<> values. This is different for each slow path function. 5. Check the Checked<> result for overflow. All this needs to be done in addition to the existing code that does the desired operation on the unshifted Int64 values, because we still need the real result of the operation. Based on that, I think it's better to stick with my solution. It has the following advantages: 1. Less code changes. 2. The overflow check does not depend on the operation being done, only the result value. 3. The algorithm is consistent with the one emitted by the snippet for the profiling in the baseline JIT on double results. 4. The profiling code can be cleanly #ifdef'ed out for non-DFG builds. The Checked<> alternative can be #ifdef'ed out too, but less cleanly. I'll add your suggested comment about the intentional imprecision.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:372 > > > >> + if (int64Val >= int52OverflowPoint) > > > > > > > > Pretty sure this is wrong. The negative overflow point doesn't have the same absolute value as the positive overflow point. Why not use CheckedMath from WTF to do the check? > > > > > > Yes, I'm aware that we might get a false positive on the negative overflow > > > (1 out of 51 bits worth of numbers), but I considered that an OK tradeoff > > > for simplicity. I was not aware of CheckedMath. Let me look into it. > > > > I see. I'm fine with your approach, if you add a comment that the > > imprecision is intentional. > > > > It may be that CheckedMath makes it easier to do it right. You'll have to > > do the int52 arithmetic using int64, so left-shift before checking. > > After looking into Checked<> math usage, I see that I would need to add the > following to the slow path functions: > 1. extract the JSValue numbers into an Int64 value. > 2. Shift the Int64 values. > 3. Convert them into Checked<> values. > 4. Do the desired operation on the Checked<> values. This is different for > each slow path function. > 5. Check the Checked<> result for overflow. > > All this needs to be done in addition to the existing code that does the > desired operation on the unshifted Int64 values, because we still need the > real result of the operation. > > Based on that, I think it's better to stick with my solution. It has the > following advantages: > 1. Less code changes. > 2. The overflow check does not depend on the operation being done, only the > result value. > 3. The algorithm is consistent with the one emitted by the snippet for the > profiling in the baseline JIT on double results. > 4. The profiling code can be cleanly #ifdef'ed out for non-DFG builds. The > Checked<> alternative can be #ifdef'ed out too, but less cleanly. > > I'll add your suggested comment about the intentional imprecision. SGTM.

Created attachment 268302 [details] proposed patch w/ iOS build fixes. The patch now builds on iOS. I'm still running tests to make sure that the added emitters are doing the right thing. This patch will break the other ports that don't have the following emitter though: void or32(TrustedImm32 imm, AbsoluteAddress address);

*** Bug 152752 has been marked as a duplicate of this bug. ***

The JSC tests runs fine on ARMv7 and ARM64. Ready for a review.

Comment on attachment 268302 [details] proposed patch w/ iOS build fixes. r=me

Thanks for the review. Landed in r194613: <http://trac.webkit.org/r194613>.

Re-opened since this is blocked by bug 152805

Comment on attachment 268302 [details] proposed patch w/ iOS build fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=268302&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:359 > + or32(imm, dataTempRegister, dataTempRegister); I think it is incorrect, because or32(TrustedImm32 imm, RegisterID src, RegisterID dest) moves the immediate to dataTempRegister and overwrite the value you loaded to it previously. (I had a similar issue in bug152784 during implementing the or32 for ARM instruction set.)

Comment on attachment 268302 [details] proposed patch w/ iOS build fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=268302&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:359 >> + or32(imm, dataTempRegister, dataTempRegister); > > I think it is incorrect, because or32(TrustedImm32 imm, RegisterID src, RegisterID dest) > moves the immediate to dataTempRegister and overwrite the value you loaded to it previously. > (I had a similar issue in bug152784 during implementing the or32 for ARM instruction set.) Funny. I knew about this issue (which is why I didn't implement it for ARM), and thought I had solved it for ARMv7. Obviously, the bug still exists. In practice, this will not be a problem for the current use case were the imm being used is always a single bit. However, there is a correctness issue here. I will fix.

The fix for the or32 emitter bug is tracked at https://bugs.webkit.org/show_bug.cgi?id=152833.

*** Bug 152752 has been marked as a duplicate of this bug. ***