112141 2013-03-12 06:01:27 -0700 LLInt CLoop backend misses Double2Ints() on 32bit architectures 2013-03-12 09:03:04 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rgabor webkit-unassigned fpizlo ossy webkit.review.bot zherczeg oldest_to_newest 853396 0 rgabor 2013-03-12 06:01:27 -0700 LLInt cloop backend misses Double2Ints() on 32bit architectures 853405 1 192723 rgabor 2013-03-12 06:19:16 -0700 Created attachment 192723 proposed fix 853494 2 192723 fpizlo 2013-03-12 08:26:05 -0700 Comment on attachment 192723 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=192723&action=review R=me, but it looks like both Ints2Double and Double2Ints are not endianness-friendly. > Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:130 > +static void Double2Ints(double val, uint32_t& lo, uint32_t& hi) > +{ > + union { > + double dval; > + uint64_t ival64; > + } u; > + u.dval = val; > + hi = static_cast<uint32_t>(u.ival64 >> 32); > + lo = static_cast<uint32_t>(u.ival64); > +} Is this right? Does Double2Ints mandate 'hi' and 'lo' being the low-order bits and the high-order bits, or are they supposed to be the first bits and the last bits? 853498 3 192723 webkit.review.bot 2013-03-12 08:30:24 -0700 Comment on attachment 192723 proposed fix Clearing flags on attachment: 192723 Committed r145551: <http://trac.webkit.org/changeset/145551> 853499 4 webkit.review.bot 2013-03-12 08:30:27 -0700 All reviewed patches have been landed. Closing bug. 853521 5 rgabor 2013-03-12 09:03:04 -0700 (In reply to comment #2) > (From update of attachment 192723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192723&action=review > > R=me, but it looks like both Ints2Double and Double2Ints are not endianness-friendly. > Nope they don't, should we make it endianness-friendly? > > Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:130 > > +static void Double2Ints(double val, uint32_t& lo, uint32_t& hi) > > +{ > > + union { > > + double dval; > > + uint64_t ival64; > > + } u; > > + u.dval = val; > > + hi = static_cast<uint32_t>(u.ival64 >> 32); > > + lo = static_cast<uint32_t>(u.ival64); > > +} > > Is this right? Does Double2Ints mandate 'hi' and 'lo' being the low-order bits and the high-order bits, or are they supposed to be the first bits and the last bits? I've just made it as the Ints2Double. We tried it on JSC tests and everything went fine. So I think its ok for the little-endian architecture at least. 192723 2013-03-12 06:19:16 -0700 2013-03-12 08:30:24 -0700 proposed fix cloop.patch text/plain 2353 rgabor ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGEzZjdlNjQuLmQ3YTAwMjMgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTAzLTEyICBHYWJvciBSYXBjc2Fu eWkgIDxyZ2Fib3JAd2Via2l0Lm9yZz4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTEyMTQxCisgICAgICAgIExMSW50IENMb29wIGJhY2tlbmQgbWlz c2VzIERvdWJsZTJJbnRzKCkgb24gMzJiaXQgYXJjaGl0ZWN0dXJlcworCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEltcGxlbWVudCBEb3VibGUySW50cygp IGluIENMb29wIGJhY2tlbmQgb2YgTExJbnQgb24gMzJiaXQgYXJjaGl0ZWN0dXJlcy4KKworICAg ICAgICAqIGxsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIuY3BwOgorICAgICAgICAoTExJbnQpOgor ICAgICAgICAoSlNDOjpMTEludDo6RG91YmxlMkludHMpOgorICAgICAgICAqIG9mZmxpbmVhc20v Y2xvb3AucmI6CisKIDIwMTMtMDMtMTIgIENzYWJhIE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0 Lm9yZz4KIAogICAgICAgICBSRUdSRVNTSU9OKHIxNDU0ODIpOiBJdCBicm9rZSAzMyBqc2MgdGVz dHMgYW5kIHppbGxpb24gbGF5b3V0IHRlc3RzIG9uIGFsbCBwbGF0Zm9ybQpkaWZmIC0tZ2l0IGEv U291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIuY3BwIGIvU291 cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXIuY3BwCmluZGV4IGQz YzczYjAuLjE4NmM2NTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGludC9M b3dMZXZlbEludGVycHJldGVyLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQv TG93TGV2ZWxJbnRlcnByZXRlci5jcHAKQEAgLTExNyw2ICsxMTcsMTcgQEAgc3RhdGljIGRvdWJs ZSBJbnRzMkRvdWJsZSh1aW50MzJfdCBsbywgdWludDMyX3QgaGkpCiAgICAgdS5pdmFsNjQgPSAo c3RhdGljX2Nhc3Q8dWludDY0X3Q+KGhpKSA8PCAzMikgfCBsbzsKICAgICByZXR1cm4gdS5kdmFs OwogfQorCitzdGF0aWMgdm9pZCBEb3VibGUySW50cyhkb3VibGUgdmFsLCB1aW50MzJfdCYgbG8s IHVpbnQzMl90JiBoaSkKK3sKKyAgICB1bmlvbiB7CisgICAgICAgIGRvdWJsZSBkdmFsOworICAg ICAgICB1aW50NjRfdCBpdmFsNjQ7CisgICAgfSB1OworICAgIHUuZHZhbCA9IHZhbDsKKyAgICBo aSA9IHN0YXRpY19jYXN0PHVpbnQzMl90Pih1Lml2YWw2NCA+PiAzMik7CisgICAgbG8gPSBzdGF0 aWNfY2FzdDx1aW50MzJfdD4odS5pdmFsNjQpOworfQogI2VuZGlmIC8vIFVTRShKU1ZBTFVFMzJf NjQpCiAKIH0gLy8gbmFtZXNwYWNlIExMaW50CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvb2ZmbGluZWFzbS9jbG9vcC5yYiBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9vZmZsaW5l YXNtL2Nsb29wLnJiCmluZGV4IDE0Y2FjYjkuLmY2YmQ1NTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9vZmZsaW5lYXNtL2Nsb29wLnJiCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9vZmZsaW5lYXNtL2Nsb29wLnJiCkBAIC0xMDI1LDcgKzEwMjUsNyBAQCBjbGFzcyBJbnN0 cnVjdGlvbgogICAgICAgICAjIDMyLWJpdCBpbnN0cnVjdGlvbjogZjJkaWkgZGJsT3AgaW50MzJM b09wIGludDMySGlPcCAoYmFzZWQgb24gQVJNdjcpCiAgICAgICAgICMgRW5jb2RlIGEgNjQtYml0 IGRvdWJsZSBpbnRvIDIgMzItYml0IGludHMgKGxvdyBhbmQgaGlnaCkuCiAgICAgICAgIHdoZW4g ImZkMmlpIgotICAgICAgICAgICAgJGFzbS5wdXRjICJEb3VibGUySW50cygje29wZXJhbmRzWzBd LmNsVmFsdWUoOmRvdWJsZSl9LCAje29wZXJhbmRzWzFdLmNsVmFsdWV9LCAje29wZXJhbmRzWzJd LmNsVmFsdWV9KTsiCisgICAgICAgICAgICAkYXNtLnB1dGMgIkRvdWJsZTJJbnRzKCN7b3BlcmFu ZHNbMF0uY2xWYWx1ZSg6ZG91YmxlKX0sICN7b3BlcmFuZHNbMV0uY2xWYWx1ZSg6dWludDMyKX0s ICN7b3BlcmFuZHNbMl0uY2xWYWx1ZSg6dWludDMyKX0pOyIKIAogICAgICAgICAjIDY0LWJpdCBp bnN0cnVjdGlvbjogZnEyZCBpbnQ2NE9wIGRibE9wIChiYXNlZCBvbiBYNjQpCiAgICAgICAgICMg Q29weSBhIGJpdC1lbmNvZGVkIGRvdWJsZSBpbiBhIDY0LWJpdCBpbnQgcmVnaXN0ZXIgdG8gYSBk b3VibGUgcmVnaXN0ZXIuCg==