226817 2021-06-09 07:21:51 -0700 [JSC] Fix incorrect register reuse in 32bit after r278568 2021-06-09 09:32:39 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 xan.lopez webkit-unassigned angelos ews-watchlist keith_miller mark.lam msaboff saam ticaiolima tzagallo webkit-bug-importer xan.lopez oldest_to_newest 1768164 0 xan.lopez 2021-06-09 07:21:51 -0700 The JSVALUE32_64 branch potentially needs both the tag and payload registers for both left/right nodes, so we cannot reuse any of them for the result since the first thing the code does is set it zero. Just remove the Reuse construction. This was causing extremely long test runs on 32bit. 1768165 1 430963 xan.lopez 2021-06-09 07:23:45 -0700 Created attachment 430963 Fix incorrect register reuse v1 1768168 2 angelos 2021-06-09 07:27:59 -0700 *** Bug 226815 has been marked as a duplicate of this bug. *** 1768171 3 430963 ticaiolima 2021-06-09 07:45:26 -0700 Comment on attachment 430963 Fix incorrect register reuse View in context: https://bugs.webkit.org/attachment.cgi?id=430963&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388 > + GPRTemporary result(this); Could we try to still reuse it, but then move 0 after `notEqual.link(&m_jit);` on line 7404? I think this would allow us to reuse left and right payload/tag there. 1768172 4 430963 ticaiolima 2021-06-09 07:47:41 -0700 Comment on attachment 430963 Fix incorrect register reuse View in context: https://bugs.webkit.org/attachment.cgi?id=430963&action=review r=me >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388 >> + GPRTemporary result(this); > > Could we try to still reuse it, but then move 0 after `notEqual.link(&m_jit);` on line 7404? I think this would allow us to reuse left and right payload/tag there. Never mind, this would require another jump for Equals case. Not reusing the tag sounds a better compromise then generating multiple branchs 1768175 5 430963 ticaiolima 2021-06-09 07:51:48 -0700 Comment on attachment 430963 Fix incorrect register reuse 32-bits queue is quite long, committing this to make it process patches 1768176 6 430963 ticaiolima 2021-06-09 08:03:15 -0700 Comment on attachment 430963 Fix incorrect register reuse I retract this change. I think it wont work because we need to set `false` to result when left.tagGPR(), right.tagGPR() are different. 1768183 7 ticaiolima 2021-06-09 08:12:55 -0700 (In reply to Caio Lima from comment #6) > Comment on attachment 430963 [details] > Fix incorrect register reuse > > I retract this change. I think it wont work because we need to set `false` > to result when left.tagGPR(), right.tagGPR() are different. The patch already does that, since it doesn’t change previous behavior. So, my previous comment is wrong. I’ll r+/cq+ again after local run shows it passes all tests. 1768184 8 430963 ticaiolima 2021-06-09 08:13:16 -0700 Comment on attachment 430963 Fix incorrect register reuse r=me 1768203 9 ews-feeder 2021-06-09 09:31:32 -0700 Committed r278662 (238644@main): <https://commits.webkit.org/238644@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430963. 1768205 10 webkit-bug-importer 2021-06-09 09:32:39 -0700 <rdar://problem/79081335> 430963 2021-06-09 07:23:45 -0700 2021-06-09 09:31:33 -0700 Fix incorrect register reuse 0001-JSC-Fix-incorrect-register-reuse-in-32bit-after-r278.patch text/plain 2433 xan.lopez RnJvbSAzYTAxZjg2M2Q0Yzg5NzEwZTMyZWMxMTVkNDcyZjU4NGIxMjYwZjkwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiA9P1VURi04P3E/WGFuPTIwTD1DMz1CM3Blej89IDx4YW5AaWdh bGlhLmNvbT4KRGF0ZTogV2VkLCA5IEp1biAyMDIxIDE2OjIyOjUyICswMjAwClN1YmplY3Q6IFtQ QVRDSF0gW0pTQ10gRml4IGluY29ycmVjdCByZWdpc3RlciByZXVzZSBpbiAzMmJpdCBhZnRlciBy Mjc4NTY4CiBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI2ODE3CgpS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCiogZGZnL0RGR1NwZWN1bGF0aXZlSklULmNwcDoK KEpTQzo6REZHOjpTcGVjdWxhdGl2ZUpJVDo6Y29tcGlsZU5vdERvdWJsZU5laXRoZXJEb3VibGVO b3JIZWFwQmlnSW50Tm9yU3RyaW5nU3RyaWN0RXF1YWxpdHkpOgpUaGUgSlNWQUxVRTMyXzY0IGJy YW5jaCBwb3RlbnRpYWxseSBuZWVkcyBib3RoIHRoZSB0YWcgYW5kIHBheWxvYWQKcmVnaXN0ZXJz IGZvciBib3RoIGxlZnQvcmlnaHQgbm9kZXMsIHNvIHdlIGNhbm5vdCByZXVzZSBhbnkgb2YKdGhl bSBmb3IgdGhlIHJlc3VsdCBzaW5jZSB0aGUgZmlyc3QgdGhpbmcgdGhlIGNvZGUgZG9lcyBpcyBz ZXQgaXQKemVyby4gSnVzdCByZW1vdmUgdGhlIFJldXNlIGNvbnN0cnVjdGlvbi4KLS0tCiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICB8IDE0ICsrKysrKysr KysrKysrCiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZlSklULmNwcCB8 ICAyICstCiAyIGZpbGVzIGNoYW5nZWQsIDE1IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkK CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBmZDk5NzQyZmYwZmYuLjNiMzM5ZmM5ZDQ3YyAx MDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMjEtMDYtMDkgIFhh biBMb3BleiAgPHhhbkBpZ2FsaWEuY29tPgorCisgICAgICAgIFtKU0NdIEZpeCBpbmNvcnJlY3Qg cmVnaXN0ZXIgcmV1c2UgaW4gMzJiaXQgYWZ0ZXIgcjI3ODU2OAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI2ODE3CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBkZmcvREZHU3BlY3VsYXRpdmVKSVQuY3Bw OgorICAgICAgICAoSlNDOjpERkc6OlNwZWN1bGF0aXZlSklUOjpjb21waWxlTm90RG91YmxlTmVp dGhlckRvdWJsZU5vckhlYXBCaWdJbnROb3JTdHJpbmdTdHJpY3RFcXVhbGl0eSk6CisgICAgICAg IFRoZSBKU1ZBTFVFMzJfNjQgYnJhbmNoIHBvdGVudGlhbGx5IG5lZWRzIGJvdGggdGhlIHRhZyBh bmQgcGF5bG9hZAorICAgICAgICByZWdpc3RlcnMgZm9yIGJvdGggbGVmdC9yaWdodCBub2Rlcywg c28gd2UgY2Fubm90IHJldXNlIGFueSBvZgorICAgICAgICB0aGVtIGZvciB0aGUgcmVzdWx0IHNp bmNlIHRoZSBmaXJzdCB0aGluZyB0aGUgY29kZSBkb2VzIGlzIHNldCBpdAorICAgICAgICB6ZXJv LiBKdXN0IHJlbW92ZSB0aGUgUmV1c2UgY29uc3RydWN0aW9uLgorCiAyMDIxLTA2LTA4ICBZdXN1 a2UgU3V6dWtpICA8eXN1enVraUBhcHBsZS5jb20+CiAKICAgICAgICAgW0pTQ10gVXNlIERhdGFJ QyBmb3IgQWNjZXNzQ2FzZQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9E RkdTcGVjdWxhdGl2ZUpJVC5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1 bGF0aXZlSklULmNwcAppbmRleCBjOGNiMjI3MDRkZTUuLjE0MTcxZDg5NjA4YSAxMDA2NDQKLS0t IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHAKKysrIGIv U291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHAKQEAgLTczODUs NyArNzM4NSw3IEBAIHZvaWQgU3BlY3VsYXRpdmVKSVQ6OmNvbXBpbGVOb3REb3VibGVOZWl0aGVy RG91YmxlTm9ySGVhcEJpZ0ludE5vclN0cmluZ1N0cmljdEVxCiAjaWYgVVNFKEpTVkFMVUU2NCkK ICAgICBHUFJUZW1wb3JhcnkgcmVzdWx0KHRoaXMsIFJldXNlLCBsZWZ0LCByaWdodCk7CiAjZWxz ZQotICAgIEdQUlRlbXBvcmFyeSByZXN1bHQodGhpcywgUmV1c2UsIGxlZnQsIFBheWxvYWRXb3Jk KTsKKyAgICBHUFJUZW1wb3JhcnkgcmVzdWx0KHRoaXMpOwogI2VuZGlmCiAgICAgSlNWYWx1ZVJl Z3MgbGVmdFJlZ3MgPSBsZWZ0LmpzVmFsdWVSZWdzKCk7CiAgICAgSlNWYWx1ZVJlZ3MgcmlnaHRS ZWdzID0gcmlnaHQuanNWYWx1ZVJlZ3MoKTsKLS0gCjIuMzEuMQoK