153693 2016-01-29 19:45:56 -0800 B3 should reduce Mod(value, constant) to Div and Mul so that our Div optimizations can do things 2016-01-30 00:56:07 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED P2 Normal --- 150507 1 fpizlo fpizlo barraclough benjamin ggaren keith_miller mark.lam mhahnenb msaboff oliver saam sam oldest_to_newest 1160389 0 fpizlo 2016-01-29 19:45:56 -0800 Patch forthcoming. 1160390 1 270286 fpizlo 2016-01-29 19:48:55 -0800 Created attachment 270286 the patch 1160396 2 270286 saam 2016-01-29 20:39:46 -0800 Comment on attachment 270286 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=270286&action=review R=me > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:726 > + m_changed = true; This indentation looks off to me (but I'm on my phone so maybe not) > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:762 > + // This does work for the D = -1 special case. Why not turn D=-1 and D=1 into 0 as the result? > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:764 > + // = -2^31 - -2^31 * -1 This should be: -2^31 - (2^31 * -1) 1160403 3 fpizlo 2016-01-29 21:05:01 -0800 (In reply to comment #2) > Comment on attachment 270286 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270286&action=review > > R=me > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:726 > > + m_changed = true; > > This indentation looks off to me (but I'm on my phone so maybe not) > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:762 > > + // This does work for the D = -1 special case. > > Why not turn D=-1 and D=1 into 0 as the result? Because I didn't know if that was correct. I guess it is. We already get this optimization since we have Sub(N,Mul(Div(N,1),1)) which reduces to Sub(N,Mul(N,1)) then Sub(N,N) and then 0. Unless the strength reduction phase becomes very expensive, it's sort of nice that it handles a lot of things by induction. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:764 > > + // = -2^31 - -2^31 * -1 > > This should be: > -2^31 - (2^31 * -1) I really did mean "-2^31 - ((-(2^31)) * -1)". Is it not clear that this is what is meant from how I wrote it? 1160406 4 fpizlo 2016-01-29 21:15:03 -0800 Landed in http://trac.webkit.org/changeset/195882 1160436 5 270286 saam 2016-01-29 23:59:46 -0800 Comment on attachment 270286 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=270286&action=review >>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:764 >>> + // = -2^31 - -2^31 * -1 >> >> This should be: >> -2^31 - (2^31 * -1) > > I really did mean "-2^31 - ((-(2^31)) * -1)". Is it not clear that this is what is meant from how I wrote it? I understood your notation, but the calculation you wrote is incorrect: -(2^31) - (-2^31 * -1) = -(2^31) - (2^31) = -2^32 because -2^31 - -2^31 / -1 * -1 !== -(2^31) - (-2^31 * -1) it is -2^31 - -2^31 / -1 * -1 = -2^31 - (2^31 * -1) = -2^31 - (-2^31) = -2^31 + 2^31 = 0 1160438 6 fpizlo 2016-01-30 00:54:49 -0800 (In reply to comment #5) > Comment on attachment 270286 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270286&action=review > > >>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:764 > >>> + // = -2^31 - -2^31 * -1 > >> > >> This should be: > >> -2^31 - (2^31 * -1) > > > > I really did mean "-2^31 - ((-(2^31)) * -1)". Is it not clear that this is what is meant from how I wrote it? > > I understood your notation, but the calculation you wrote is incorrect: > -(2^31) - (-2^31 * -1) = -(2^31) - (2^31) = -2^32 > because > -2^31 - -2^31 / -1 * -1 !== -(2^31) - (-2^31 * -1) > it is > -2^31 - -2^31 / -1 * -1 = -2^31 - (2^31 * -1) = -2^31 - (-2^31) = -2^31 + > 2^31 = 0 This is int32 math. In int32 math, -1 * -2^31 = -2*31. 1160439 7 fpizlo 2016-01-30 00:56:07 -0800 (In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 270286 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270286&action=review > > > > >>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:764 > > >>> + // = -2^31 - -2^31 * -1 > > >> > > >> This should be: > > >> -2^31 - (2^31 * -1) > > > > > > I really did mean "-2^31 - ((-(2^31)) * -1)". Is it not clear that this is what is meant from how I wrote it? > > > > I understood your notation, but the calculation you wrote is incorrect: > > -(2^31) - (-2^31 * -1) = -(2^31) - (2^31) = -2^32 > > because > > -2^31 - -2^31 / -1 * -1 !== -(2^31) - (-2^31 * -1) > > it is > > -2^31 - -2^31 / -1 * -1 = -2^31 - (2^31 * -1) = -2^31 - (-2^31) = -2^31 + > > 2^31 = 0 > > This is int32 math. In int32 math, -1 * -2^31 = -2*31. Also -2^31 / -1 = -2^31. And 0 - -2^31 = -2^31. And -(-2^31) = -2^31. 270286 2016-01-29 19:48:55 -0800 2016-01-29 20:39:46 -0800 the patch blah.patch text/plain 5537 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTk1ODgxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBA CisyMDE2LTAxLTI5ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg QjMgc2hvdWxkIHJlZHVjZSBNb2QodmFsdWUsIGNvbnN0YW50KSB0byBEaXYgYW5kIE11bCBzbyB0 aGF0IG91ciBEaXYgb3B0aW1pemF0aW9ucyBjYW4gZG8gdGhpbmdzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTM2OTMKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgbW9zdCBlZmZpY2llbnQgd2F5IHRv IGhhbmRsZSBNb2QodmFsdWUsIGNvbnN0YW50KSBpcyB0byByZWR1Y2UgaXQgdG8KKyAgICAgICAg U3ViKHZhbHVlLCBNdWwoRGl2KHZhbHVlLCBjb25zdGFudCksIGNvbnN0YW50KSkgYW5kIHRoZW4g bGV0IHRoZSBEaXYgb3B0aW1pemF0aW9ucyBkbyB0aGVpcgorICAgICAgICB0aGluZy4KKworICAg ICAgICBJbiB0aGUgZnV0dXJlIHdlIGNvdWxkIGFkZCBzcGVjaWFsIGhhbmRsaW5nIG9mIE1vZCh2 YWx1ZSwgMSA8PCBjb25zdGFudCksIGJ1dCBpdCdzIG5vdAorICAgICAgICBvYnZpb3VzIHRoYXQg dGhpcyB3b3VsZCBwcm9kdWNlIGJldHRlciBjb2RlIHRoYW4gcmVkdWNpbmcgdGhyb3VnaCBEaXYs IGlmIHdlIGFsc28gbWFrZSBzdXJlCisgICAgICAgIHRoYXQgd2UgaGF2ZSBncmVhdCBvcHRpbWl6 YXRpb25zIGZvciBNdWwgYW5kIERpdi4KKworICAgICAgICAqIGIzL0IzUmVkdWNlU3RyZW5ndGgu Y3BwOgorCiAyMDE2LTAxLTI5ICBLZWl0aCBNaWxsZXIgIDxrZWl0aF9taWxsZXJAYXBwbGUuY29t PgogCiAgICAgICAgIEFycmF5LnByb3RvdHlwZSBuYXRpdmUgZnVuY3Rpb25zIHNob3VsZCB1c2Ug U3ltYm9sLnNwZWNpZXMgdG8gY29uc3RydWN0IHRoZSByZXN1bHQKSW5kZXg6IFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9iMy9CM1JlZHVjZVN0cmVuZ3RoLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvYjMvQjNSZWR1Y2VTdHJlbmd0aC5jcHAJKHJldmlzaW9uIDE5NTg3NykK KysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM1JlZHVjZVN0cmVuZ3RoLmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtNjYyLDE0ICs2NjIsMTIgQEAgcHJpdmF0ZToKICAgICAgICAgICAgICAgICAg ICAgLy8gV2UgY2FuIGRvIHRoaXMgYmVjYXVzZSBpdCdzIHByZWNpc2VseSBjb3JyZWN0IGZvciBD aGlsbERpdiBhbmQgZm9yIERpdiB3ZQogICAgICAgICAgICAgICAgICAgICAvLyBhcmUgYWxsb3dl ZCB0byBkbyB3aGF0ZXZlciB3ZSB3YW50LgogICAgICAgICAgICAgICAgICAgICBtX3ZhbHVlLT5y ZXBsYWNlV2l0aElkZW50aXR5KG1fdmFsdWUtPmNoaWxkKDEpKTsKLSAgICAgICAgICAgICAgICAg ICAgbV9jaGFuZ2VkID0gdHJ1ZTsKICAgICAgICAgICAgICAgICAgICAgYnJlYWs7CiAKICAgICAg ICAgICAgICAgICBjYXNlIDE6CiAgICAgICAgICAgICAgICAgICAgIC8vIFR1cm4gdGhpczogRGl2 KHZhbHVlLCAxKQogICAgICAgICAgICAgICAgICAgICAvLyBJbnRvIHRoaXM6IHZhbHVlCiAgICAg ICAgICAgICAgICAgICAgIG1fdmFsdWUtPnJlcGxhY2VXaXRoSWRlbnRpdHkobV92YWx1ZS0+Y2hp bGQoMCkpOwotICAgICAgICAgICAgICAgICAgICBtX2NoYW5nZWQgPSB0cnVlOwogICAgICAgICAg ICAgICAgICAgICBicmVhazsKIAogICAgICAgICAgICAgICAgIGRlZmF1bHQ6CkBAIC03MjIsMTIg KzcyMCwxMSBAQCBwcml2YXRlOgogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBtX2lu ZGV4LCBaU2hyLCBtX3ZhbHVlLT5vcmlnaW4oKSwgbWFnaWNRdW90aWVudCwKICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgbV9pbnNlcnRpb25TZXQuaW5zZXJ0PENvbnN0MzJWYWx1ZT4o CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBtX2luZGV4LCBtX3ZhbHVlLT5v cmlnaW4oKSwgMzEpKSkpOwotICAgICAgICAgICAgICAgICAgICBtX2NoYW5nZWQgPSB0cnVlOwog ICAgICAgICAgICAgICAgICAgICBicmVhazsKICAgICAgICAgICAgICAgICB9CiAKLSAgICAgICAg ICAgICAgICBpZiAobV92YWx1ZS0+b3Bjb2RlKCkgIT0gQ2hpbGxEaXYgJiYgbV92YWx1ZS0+b3Bj b2RlKCkgIT0gRGl2KQotICAgICAgICAgICAgICAgICAgICBicmVhazsKKyAgICAgICAgICAgICAg ICBtX2NoYW5nZWQgPSB0cnVlOworICAgICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICAgICAg fQogICAgICAgICAgICAgYnJlYWs7CiAKQEAgLTczNyw2ICs3MzQsNjggQEAgcHJpdmF0ZToKICAg ICAgICAgICAgIC8vIEludG8gdGhpczogY29uc3RhbnQxIC8gY29uc3RhbnQyCiAgICAgICAgICAg ICAvLyBOb3RlIHRoYXQgdGhpcyB1c2VzIENoaWxsTW9kIHNlbWFudGljcy4KICAgICAgICAgICAg IHJlcGxhY2VXaXRoTmV3VmFsdWUobV92YWx1ZS0+Y2hpbGQoMCktPm1vZENvbnN0YW50KG1fcHJv YywgbV92YWx1ZS0+Y2hpbGQoMSkpKTsKKworICAgICAgICAgICAgLy8gTW9kdWxvIGJ5IGNvbnN0 YW50IGlzIG1vcmUgZWZmaWNpZW50IGlmIHdlIHR1cm4gaXQgaW50byBEaXYsIGFuZCB0aGVuIGxl dCBEaXYgZ2V0CisgICAgICAgICAgICAvLyBvcHRpbWl6ZWQuCisgICAgICAgICAgICBpZiAobV92 YWx1ZS0+Y2hpbGQoMSktPmhhc0ludCgpKSB7CisgICAgICAgICAgICAgICAgc3dpdGNoIChtX3Zh bHVlLT5jaGlsZCgxKS0+YXNJbnQoKSkgeworICAgICAgICAgICAgICAgIGNhc2UgMDoKKyAgICAg ICAgICAgICAgICAgICAgLy8gVHVybiB0aGlzOiBNb2QodmFsdWUsIDApCisgICAgICAgICAgICAg ICAgICAgIC8vIEludG8gdGhpczogMAorICAgICAgICAgICAgICAgICAgICAvLyBUaGlzIGlzIGNv cnJlY3QgYWNjb3JkaW5nIHRvIENoaWxsTW9kIHNlbWFudGljcy4KKyAgICAgICAgICAgICAgICAg ICAgbV92YWx1ZS0+cmVwbGFjZVdpdGhJZGVudGl0eShtX3ZhbHVlLT5jaGlsZCgxKSk7CisgICAg ICAgICAgICAgICAgICAgIGJyZWFrOworCisgICAgICAgICAgICAgICAgZGVmYXVsdDoKKyAgICAg ICAgICAgICAgICAgICAgLy8gVHVybiB0aGlzOiBNb2QoTiwgRCkKKyAgICAgICAgICAgICAgICAg ICAgLy8gSW50byB0aGlzOiBTdWIoTiwgTXVsKERpdihOLCBEKSwgRCkpCisgICAgICAgICAgICAg ICAgICAgIC8vCisgICAgICAgICAgICAgICAgICAgIC8vIFRoaXMgaXMgYSBzcGVlZC11cCBiZWNh dXNlIHdlIHVzZSBvdXIgZXhpc3RpbmcgRGl2IG9wdGltaXphdGlvbnMuCisgICAgICAgICAgICAg ICAgICAgIC8vCisgICAgICAgICAgICAgICAgICAgIC8vIEhlcmUncyBhbiBlYXNpZXIgd2F5IHRv IGxvb2sgYXQgaXQ6CisgICAgICAgICAgICAgICAgICAgIC8vICAgICBOICUgRCA9IE4gLSBOIC8g RCAqIEQKKyAgICAgICAgICAgICAgICAgICAgLy8KKyAgICAgICAgICAgICAgICAgICAgLy8gTm90 ZSB0aGF0IHRoaXMgZG9lcyBub3Qgd29yayBmb3IgRCA9IDAgYW5kIENoaWxsTW9kLiBUaGUgZXhw ZWN0ZWQgcmVzdWx0IGlzIDAuCisgICAgICAgICAgICAgICAgICAgIC8vIFRoYXQncyB3aHkgd2Ug aGF2ZSBhIHNwZWNpYWwtY2FzZSBhYm92ZS4KKyAgICAgICAgICAgICAgICAgICAgLy8gICAgIFgg JSAwID0gWCAtIFggLyAwICogMCA9IFggICAgIChzaG91bGQgYmUgMCkKKyAgICAgICAgICAgICAg ICAgICAgLy8KKyAgICAgICAgICAgICAgICAgICAgLy8gVGhpcyBkb2VzIHdvcmsgZm9yIHRoZSBE ID0gLTEgc3BlY2lhbCBjYXNlLgorICAgICAgICAgICAgICAgICAgICAvLyAgICAgLTJeMzEgJSAt MSA9IC0yXjMxIC0gLTJeMzEgLyAtMSAqIC0xCisgICAgICAgICAgICAgICAgICAgIC8vICAgICAg ICAgICAgICAgID0gLTJeMzEgLSAtMl4zMSAqIC0xCisgICAgICAgICAgICAgICAgICAgIC8vICAg ICAgICAgICAgICAgID0gLTJeMzEgLSAtMl4zMQorICAgICAgICAgICAgICAgICAgICAvLyAgICAg ICAgICAgICAgICA9IDAKKworICAgICAgICAgICAgICAgICAgICBPcGNvZGUgZGl2T3Bjb2RlOwor ICAgICAgICAgICAgICAgICAgICBzd2l0Y2ggKG1fdmFsdWUtPm9wY29kZSgpKSB7CisgICAgICAg ICAgICAgICAgICAgIGNhc2UgTW9kOgorICAgICAgICAgICAgICAgICAgICAgICAgZGl2T3Bjb2Rl ID0gRGl2OworICAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAgICAgICAgICAg ICAgIGNhc2UgQ2hpbGxNb2Q6CisgICAgICAgICAgICAgICAgICAgICAgICBkaXZPcGNvZGUgPSBD aGlsbERpdjsKKyAgICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAg ICAgICBkZWZhdWx0OgorICAgICAgICAgICAgICAgICAgICAgICAgZGl2T3Bjb2RlID0gT29wczsK KyAgICAgICAgICAgICAgICAgICAgICAgIFJFTEVBU0VfQVNTRVJUX05PVF9SRUFDSEVEKCk7Cisg ICAgICAgICAgICAgICAgICAgICAgICBicmVhazsKKyAgICAgICAgICAgICAgICAgICAgfQorCisg ICAgICAgICAgICAgICAgICAgIG1fdmFsdWUtPnJlcGxhY2VXaXRoSWRlbnRpdHkoCisgICAgICAg ICAgICAgICAgICAgICAgICBtX2luc2VydGlvblNldC5pbnNlcnQ8VmFsdWU+KAorICAgICAgICAg ICAgICAgICAgICAgICAgICAgIG1faW5kZXgsIFN1YiwgbV92YWx1ZS0+b3JpZ2luKCksCisgICAg ICAgICAgICAgICAgICAgICAgICAgICAgbV92YWx1ZS0+Y2hpbGQoMCksCisgICAgICAgICAgICAg ICAgICAgICAgICAgICAgbV9pbnNlcnRpb25TZXQuaW5zZXJ0PFZhbHVlPigKKyAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgbV9pbmRleCwgTXVsLCBtX3ZhbHVlLT5vcmlnaW4oKSwKKyAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9pbnNlcnRpb25TZXQuaW5zZXJ0PFZhbHVl PigKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG1faW5kZXgsIGRpdk9wY29k ZSwgbV92YWx1ZS0+b3JpZ2luKCksCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICBtX3ZhbHVlLT5jaGlsZCgwKSwgbV92YWx1ZS0+Y2hpbGQoMSkpLAorICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICBtX3ZhbHVlLT5jaGlsZCgxKSkpKTsKKyAgICAgICAgICAgICAgICAg ICAgYnJlYWs7CisgICAgICAgICAgICAgICAgfQorICAgICAgICAgICAgICAgIAorICAgICAgICAg ICAgICAgIG1fY2hhbmdlZCA9IHRydWU7CisgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAg ICAgICB9CisgICAgICAgICAgICAKICAgICAgICAgICAgIGJyZWFrOwogCiAgICAgICAgIGNhc2Ug Qml0QW5kOgo=