Created attachment 416996 [details] Patch

Comment on attachment 416996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review > Source/JavaScriptCore/ChangeLog:8 > + `array.filter(Boolean)` is a rather popular idiom for compacting an array (removing falsy items). For example https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_compact > Source/JavaScriptCore/ChangeLog:18 > + This change advances provided microbenchmark by 110%, and is neutral for other ToBoolean cases. r270932 patch array-filter-boolean-constructor 130.0720+-2.7747 ^ 61.4302+-1.1755 ^ definitely 2.1174x faster

Comment on attachment 416996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1803 > +void SpeculativeJIT::compileToBooleanObjectOrOther(Edge nodeUse, bool invert) While this function cares about masqueradesAsUndefined, I don't think this is necessary for Boolean constructor. Can you change the following things? 1. Let's change ToBoolean DFG node to CallBooleanConstructor node to make it more explicit that this is boolean constructor call (ToBoolean sounds like we should care masqueradesAsUndefined too). 2. Let's remove masqueradesAsUndefined handlings for CallBooleanConstructor.

(In reply to Yusuke Suzuki from comment #3) > While this function cares about masqueradesAsUndefined, I don't think this > is necessary for Boolean constructor. Sadly, it is necessary: Annex B for [[IsHTMLDDA]] modifies ToBoolean operation itself: https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot-to-boolean. Chrome and Firefox follow it for `Boolean(document.all) => false`, as does this patch. Updated stress tests do cover masqueraders, I will make sure to note this in ChangeLog.

Oh, wow! Fun…

Comment on attachment 416996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review Looks good. But putting r- since I found several bugs. > Source/JavaScriptCore/ChangeLog:20 > + * dfg/DFGAbstractInterpreterInlines.h: Yeah, please add notes about masquerade as undefined thing in ChangeLog, and please add tests for that :) That's awesome. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1538 > GPRTemporary result(this, Reuse, value); > - m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); > + if (invert) > + m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); > booleanResult(result.gpr(), node); Reuse is "Reuse if it is possible" If node->child1's use count is not zero at this point, GPRTemporary allocates a new register. So, if you do not move value to result, result can contain a garbage. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1905 > GPRTemporary result(this, Reuse, value); While this is old code, I wonder if SpeculateBooleanOperand is correct here. Can you check what format is generated from SpeculateBooleanOperand? JSValue? Or 0/1? Maybe, in 64bit, we never use DataFormatBoolean in DFG? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1924 > + if (invert) > + m_jit.xor64(TrustedImm32(JSValue::ValueTrue), result.gpr()); Is it correct? If `invert` is false, is result the JSValue format? I think result is 0/1 at this point, and it is not JSBoolean.

<rdar://problem/73036835>

Created attachment 422080 [details] Patch Move value into Reuse register, fix !invert case of BooleanUse, improve tests, and add note on masquerader to ChangeLog.

(In reply to Yusuke Suzuki from comment #6) Thank you for meticulous review, Yusuke! > Yeah, please add notes about masquerade as undefined thing in ChangeLog, and > please add tests for that :) Added. > Reuse is "Reuse if it is possible" If node->child1's use count is not zero > at this point, GPRTemporary allocates a new register. > So, if you do not move value to result, result can contain a garbage. Fixed by emitting a `mov`, as we do in 64-bit version. > While this is old code, I wonder if SpeculateBooleanOperand is correct here. > Can you check what format is generated from SpeculateBooleanOperand? > JSValue? Or 0/1? I can confirm it is JSValue, so XOR against 1 is correct. > Maybe, in 64bit, we never use DataFormatBoolean in DFG? That seems to be the case. > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1924 > > + if (invert) > > + m_jit.xor64(TrustedImm32(JSValue::ValueTrue), result.gpr()); > > Is it correct? If `invert` is false, is result the JSValue format? I think > result is 0/1 at this point, and it is not JSBoolean. Yeah, it was 0/1, and non-invert case was not covered by the tests. I've updated the coverage so it now crashes with previous patch, and fixed to XOR against JSValue::ValueFalse.

Comment on attachment 422080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422080&action=review r=me > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1539 > + m_jit.move(value.gpr(), result.gpr()); > + if (invert) > + m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); Maybe, if (invert) m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); else m_jit.move(value.gpr(), result.gpr()); is clearer. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1909 > m_jit.move(value.gpr(), result.gpr()); > - m_jit.xor64(TrustedImm32(true), result.gpr()); > + if (invert) > + m_jit.xor64(TrustedImm32(1), result.gpr()); Ditto. if (invert) m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); else m_jit.move(value.gpr(), result.gpr());

Committed r274037 (234973@main): <https://commits.webkit.org/234973@main>

(In reply to Yusuke Suzuki from comment #10) > Maybe, > > if (invert) > m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); > else > m_jit.move(value.gpr(), result.gpr()); > > is clearer. Nice suggestion, landed. I wonder if there might be a bug because 32-bit compileToBoolean() has no path with type check for BooleanUse?

(In reply to Alexey Shvayka from comment #12) > (In reply to Yusuke Suzuki from comment #10) > > Maybe, > > > > if (invert) > > m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr()); > > else > > m_jit.move(value.gpr(), result.gpr()); > > > > is clearer. > > Nice suggestion, landed. > > I wonder if there might be a bug because 32-bit compileToBoolean() has no > path with type check for BooleanUse? Isn't it checking the type with SpeculateBooleanOperand?