Bug 221927

Summary: BytecodeGenerator::fuseCompareAndJump() fails for some language constructs
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Minor CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch for landing none

Alexey Shvayka
Reported 2021-02-15 14:42:13 PST
BytecodeGenerator::fuseCompareAndJump() fails for some language constructs
Attachments
Patch (18.64 KB, patch)
2021-02-15 14:45 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch for landing (20.25 KB, patch)
2021-03-01 06:00 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Shvayka
Comment 1 2021-02-15 14:45:45 PST
Created attachment 420376 [details] Patch
Yusuke Suzuki
Comment 2 2021-02-15 19:54:19 PST
Comment on attachment 420376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420376&action=review r=me, awesome! > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5176 > + emitJumpIfTrue(emitEqualityOp<OpBeloweq>(newTemporary(), context.completionTypeRegister(), emitLoad(nullptr, CompletionType::Throw)), isThrowOrNormalLabel.get()); I think emitEqualityOp is not correct for OpBeloweq (emitBinaryOp?). Could you also insert static_assert for them in emitEqualityOp? (static_assert for EqOp is StrictEq etc.).
Radar WebKit Bug Importer
Comment 3 2021-02-22 14:43:12 PST
<rdar://problem/74614681>
Alexey Shvayka
Comment 4 2021-03-01 06:00:12 PST
Created attachment 421814 [details] Patch for landing
Alexey Shvayka
Comment 5 2021-03-01 06:15:05 PST
(In reply to Yusuke Suzuki from comment #2) > r=me, awesome! I always appreciate your thorough reviews. Thanks Yusuke! > I think emitEqualityOp is not correct for OpBeloweq (emitBinaryOp?). > Could you also insert static_assert for them in emitEqualityOp? > (static_assert for EqOp is StrictEq etc.). Great catch! I've added a static_assert only for op_eq / op_stricteq because op_neq / op_nstricteq are handled in BinaryOpNode::emitBytecode() and never get passed to emitEqualityOp().
EWS
Comment 6 2021-03-01 07:00:26 PST
Committed r273649: <https://commits.webkit.org/r273649> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421814 [details].
Yusuke Suzuki
Comment 7 2021-03-02 18:08:16 PST
This improved JetStream2/Air by 5%. Awesome!
Note You need to log in before you can comment on or make changes to this bug.