113146 2013-03-23 20:38:29 -0700 Offlineasm cloop backend compiles op+branch incorrectly 2013-03-25 18:11:33 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 fpizlo mark.lam fpizlo jbriance oldest_to_newest 862301 0 fpizlo 2013-03-23 20:38:29 -0700 This is almost certainly a benign bug but it would be worth fixing. Quoth Julien: sc.rb unless there is a strong reason not to. > I started to look at risc.rb and there are some things that I don't understand. > > For instance the riscLowerSimpleBranchOps, where we have something like this: > # baddiz foo, bar, baz > # > # will become: > # > # addi foo, bar > # bz baz > > As I see in the C loop impl, I thought it would be something like: > # baddiz foo, bar, baz > # > # will become: > # > # addi foo, bar, tmp > # bz baz > # move tmp, bar > > Am I missing something here ? I confirmed by inspection that this is indeed what cloop does, in which case it's wrong. I expect this to be asymptommatic for now since if the LLInt uses those ops, it will be branching to a place where 'bar' is dead. 863189 1 mark.lam 2013-03-25 17:11:58 -0700 I asked and Filip answered: Me: I take it that the bug reported here is that the result should be applied before the branch, and is not conditional on the branch. Filip: Yup. Me: I presume that this should be true of the following opcodes also? baddio, bsubio, bmulio, baddis, baddinz, baddqs, baddqz, baddqnz, baddps, baddpz, baddpnz, bsubis, bsubiz, bsubinz, borris, borriz, borrinz. Filip: Yup. 863195 2 194958 mark.lam 2013-03-25 17:17:03 -0700 Created attachment 194958 the patch. 863210 3 194958 ggaren 2013-03-25 17:26:36 -0700 Comment on attachment 194958 the patch. r=me Is there a way to regression test for this, when the c loop happens to be enabled? 863222 4 mark.lam 2013-03-25 17:39:03 -0700 (In reply to comment #3) > Is there a way to regression test for this, when the c loop happens to be enabled? I'll think about it and file a bug to do this later if it's possible (with reasonable effort). Thanks for the review. Landed in r146831: <http://trac.webkit.org/changeset/146831>. 863240 5 mark.lam 2013-03-25 18:11:33 -0700 (In reply to comment #4) > (In reply to comment #3) > > Is there a way to regression test for this, when the c loop happens to be enabled? > > I'll think about it and file a bug to do this later if it's possible (with reasonable effort). After inspecting the llint asm code, I don't think there is any JS level test that can check to ensure that the operations is done before the branch checks (at least at this time). This is because the only existing uses of these instructions are "baddis" which is only when JIT_ENABLED (hence, not applicable to the cloop), and the branch on integer overflow cases. In the integer overflow cases, the overflow condition will result in a branch to slow code which redo the operation using doubles. Hence, we won't be able to observe that the instruction did not store the result of the integer operation. 194958 2013-03-25 17:17:03 -0700 2013-03-25 17:26:35 -0700 the patch. bug-113146.patch text/plain 3669 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTQ2ODI3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBA CisyMDEzLTAzLTI1ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBP ZmZsaW5lYXNtIGNsb29wIGJhY2tlbmQgY29tcGlsZXMgb3ArYnJhbmNoIGluY29ycmVjdGx5Lgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTEzMTQ2Lgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZGZnL0RG R1JlcGF0Y2guaDoKKyAgICAgICAgKEpTQzo6REZHOjpkZmdSZXNldEdldEJ5SUQpOgorICAgICAg ICAoSlNDOjpERkc6OmRmZ1Jlc2V0UHV0QnlJRCk6CisgICAgICAgIC0gVGhlc2UgZnVuY3Rpb25z IG5ldmVyIHJldHVybiB3aGVuIHRoZSBERkcgaXMgZHNpYWJsZWQsIG5vdCBqdXN0IHdoZW4KKyAg ICAgICAgICBhc3NlcnRzIGFyZSBlbmFibGVkLiBDaGFuZ2luZyB0aGUgYXR0cmlidXRlIGZyb20g Tk9fUkVUVVJOX0RVRV9UT19BU1NFUlQKKyAgICAgICAgICB0byBOT19SRVRVUk4uCisgICAgICAg ICogbGxpbnQvTExJbnRPZmZsaW5lQXNtQ29uZmlnLmg6CisgICAgICAgIC0gQWRkZWQgc29tZSAj ZGVmaW5lcyBuZWVkZWQgdG8gZ2V0IHRoZSBjbG9vcCBidWlsZGluZyBhZ2Fpbi4KKyAgICAgICAg KiBvZmZsaW5lYXNtL2Nsb29wLnJiOgorICAgICAgICAtIEZpeCBjbG9vcEVtaXRPcEFuZEJyYW5j aElmT3ZlcmZsb3coKSBhbmQgY2xvb3BFbWl0T3BBbmRCcmFuY2goKSB0bworICAgICAgICAgIGVt aXQgY29kZSB0aGF0IHVuY29uZGl0aW9uYWxseSBleGVjdXRlcyB0aGUgc3BlY2lmaWVkIG9wZXJh dGlvbiBiZWZvcmUKKyAgICAgICAgICBkb2luZyB0aGUgY29uZGl0aW9uYWwgYnJhbmNoLgorCiAy MDEzLTAzLTIzICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CiAKICAg ICAgICAgSGFuZGxlU2V0IHNob3VsZCB1c2UgSGVhcEJsb2NrcyBmb3Igc3RvcmluZyBoYW5kbGVz CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1JlcGF0Y2guaAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1JlcGF0Y2guaAkocmV2aXNpb24gMTQ2 ODE2KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdSZXBhdGNoLmgJKHdvcmtpbmcg Y29weSkKQEAgLTU5LDggKzU5LDggQEAgc3RydWN0IFN0cnVjdHVyZVN0dWJJbmZvOwogCiBuYW1l c3BhY2UgREZHIHsKIAotaW5saW5lIE5PX1JFVFVSTl9EVUVfVE9fQVNTRVJUIHZvaWQgZGZnUmVz ZXRHZXRCeUlEKFJlcGF0Y2hCdWZmZXImLCBTdHJ1Y3R1cmVTdHViSW5mbyYpIHsgUkVMRUFTRV9B U1NFUlRfTk9UX1JFQUNIRUQoKTsgfQotaW5saW5lIE5PX1JFVFVSTl9EVUVfVE9fQVNTRVJUIHZv aWQgZGZnUmVzZXRQdXRCeUlEKFJlcGF0Y2hCdWZmZXImLCBTdHJ1Y3R1cmVTdHViSW5mbyYpIHsg UkVMRUFTRV9BU1NFUlRfTk9UX1JFQUNIRUQoKTsgfQoraW5saW5lIE5PX1JFVFVSTiB2b2lkIGRm Z1Jlc2V0R2V0QnlJRChSZXBhdGNoQnVmZmVyJiwgU3RydWN0dXJlU3R1YkluZm8mKSB7IFJFTEVB U0VfQVNTRVJUX05PVF9SRUFDSEVEKCk7IH0KK2lubGluZSBOT19SRVRVUk4gdm9pZCBkZmdSZXNl dFB1dEJ5SUQoUmVwYXRjaEJ1ZmZlciYsIFN0cnVjdHVyZVN0dWJJbmZvJikgeyBSRUxFQVNFX0FT U0VSVF9OT1RfUkVBQ0hFRCgpOyB9CiAKIH0gfSAvLyBuYW1lc3BhY2UgSlNDOjpERkcKIApJbmRl eDogU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xMSW50T2ZmbGluZUFzbUNvbmZpZy5oCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGludC9MTEludE9mZmxpbmVBc21D b25maWcuaAkocmV2aXNpb24gMTQ2ODE2KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50 L0xMSW50T2ZmbGluZUFzbUNvbmZpZy5oCSh3b3JraW5nIGNvcHkpCkBAIC0zNSw3ICszNSw5IEBA CiAjaWYgRU5BQkxFKExMSU5UX0NfTE9PUCkKICNkZWZpbmUgT0ZGTElORV9BU01fQ19MT09QIDEK ICNkZWZpbmUgT0ZGTElORV9BU01fWDg2IDAKKyNkZWZpbmUgT0ZGTElORV9BU01fQVJNIDAKICNk ZWZpbmUgT0ZGTElORV9BU01fQVJNdjcgMAorI2RlZmluZSBPRkZMSU5FX0FTTV9BUk12N19UUkFE SVRJT05BTCAwCiAjZGVmaW5lIE9GRkxJTkVfQVNNX1g4Nl82NCAwCiAjZGVmaW5lIE9GRkxJTkVf QVNNX0FSTXY3cyAwCiAjZGVmaW5lIE9GRkxJTkVfQVNNX01JUFMgMApJbmRleDogU291cmNlL0ph dmFTY3JpcHRDb3JlL29mZmxpbmVhc20vY2xvb3AucmIKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0ph dmFTY3JpcHRDb3JlL29mZmxpbmVhc20vY2xvb3AucmIJKHJldmlzaW9uIDE0NjgxNikKKysrIFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9vZmZsaW5lYXNtL2Nsb29wLnJiCSh3b3JraW5nIGNvcHkpCkBA IC00NjUsOSArNDY1LDkgQEAgZGVmIGNsb29wRW1pdE9wQW5kQnJhbmNoKG9wZXJhbmRzLCBvcGVy YQogCiAgICAgJGFzbS5wdXRjICJ7IgogICAgICRhc20ucHV0YyAiICAgICN7dGVtcFR5cGV9IHRl bXAgPSAje29wMn0gI3tvcGVyYXRvcn0gI3tvcDF9OyIKKyAgICAkYXNtLnB1dGMgIiAgICAje29w Mn0gPSB0ZW1wOyIKICAgICAkYXNtLnB1dGMgIiAgICBpZiAodGVtcCAje2NvbmRpdGlvblRlc3R9 KSIKICAgICAkYXNtLnB1dGMgIiAgICAgICAgZ290byAgI3tvcGVyYW5kc1syXS5jTGFiZWx9OyIK LSAgICAkYXNtLnB1dGMgIiAgICAje29wMn0gPSB0ZW1wOyIKICAgICAkYXNtLnB1dGMgIn0iCiBl bmQKIApAQCAtNTMzLDEwICs1MzMsMTAgQEAgZGVmIGNsb29wRW1pdE9wQW5kQnJhbmNoSWZPdmVy ZmxvdyhvcGVyYQogICAgICAgICByYWlzZSAiVW5pbXBsZW1lbnRlZCBvcGVhcnRvciIKICAgICBl bmQKIAotICAgICRhc20ucHV0YyAiICAgIGlmICN7b3ZlcmZsb3dUZXN0fSB7IgotICAgICRhc20u cHV0YyAiICAgICAgICBnb3RvICN7b3BlcmFuZHNbMl0uY0xhYmVsfTsiCi0gICAgJGFzbS5wdXRj ICIgICAgfSIKKyAgICAkYXNtLnB1dGMgIiAgICBib29sIGRpZE92ZXJmbG93ID0gI3tvdmVyZmxv d1Rlc3R9OyIKICAgICAkYXNtLnB1dGMgIiAgICAje29wZXJhbmRzWzFdLmNsVmFsdWUodHlwZSl9 ID0gI3tvcGVyYW5kc1sxXS5jbFZhbHVlKHR5cGUpfSAje29wZXJhdG9yfSAje29wZXJhbmRzWzBd LmNsVmFsdWUodHlwZSl9OyIKKyAgICAkYXNtLnB1dGMgIiAgICBpZiAoZGlkT3ZlcmZsb3cpIgor ICAgICRhc20ucHV0YyAiICAgICAgICBnb3RvICN7b3BlcmFuZHNbMl0uY0xhYmVsfTsiCiAgICAg JGFzbS5wdXRjICJ9IgogZW5kCiAK