Summary: | [ARM64] static_cast<int32_t>() in BinaryOpNode::emitBytecode() prevents op_unsigned emission | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | buildbot, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 108645 | ||||||||
Attachments: |
|
Description
Zan Dobersek
2017-10-17 00:33:58 PDT
Identical problem also appears on ARM Thumb2, with the float value being converted to the same integer value as on ARM64.
Here's the generated code that performs conversion:
> 2352c8: ed98 7b06 vldr d7, [r8, #24]
> ...
> 2352d2: eefd 6bc7 vcvt.s32.f64 s13, d7go
Created attachment 324106 [details]
Patch
Can confirm this fixes issues on all ARM flavors. Thanks! I'll yield the review to JSC devs. Comment on attachment 324106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324106&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998 > + if (value.isInt32()) { > + if (value.asInt32() >= 0) Nit: these two if-statements can be folded into one. Comment on attachment 324106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324106&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998 >> + if (value.asInt32() >= 0) > > Nit: these two if-statements can be folded into one. Nice, fixed. Created attachment 324107 [details]
Patch
Comment on attachment 324107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324107&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997 > + auto value = jsNumber(static_cast<NumberNode*>(node)->value()); > + if (value.isInt32() && value.asInt32() >= 0) I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something? Comment on attachment 324107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324107&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997 >> + if (value.isInt32() && value.asInt32() >= 0) > > I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something? No. The goal of this is returning `UInt32Result::Constant` if the constant integer value is within uint32_t range. (this is `op_unsigned(lhs) < rhsConstantUInt32`'s rhsConstantUInt32 detection.) In this patch, we allow [0, INT32_MAX] for this case. Comment on attachment 324107 [details]
Patch
Thank you
Comment on attachment 324107 [details] Patch Clearing flags on attachment: 324107 Committed r223745: <https://trac.webkit.org/changeset/223745> All reviewed patches have been landed. Closing bug. |