150777 2015-11-01 11:40:45 -0800 Consider still matching an address expression even if B3 has already assigned a Tmp to it 2015-12-10 19:42:31 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED P2 Normal --- 152106 150279 1 fpizlo fpizlo barraclough benjamin commit-queue ggaren keith_miller mark.lam mhahnenb msaboff nrotem oliver saam sam oldest_to_newest 1138335 0 fpizlo 2015-11-01 11:40:45 -0800 Probably if we want to do hoisting of address expressions, we should have some better heuristics for it. 1148215 1 267111 fpizlo 2015-12-10 09:16:24 -0800 Created attachment 267111 the patch 1148311 2 267111 ggaren 2015-12-10 15:34:51 -0800 Comment on attachment 267111 the patch I would have guessed 2. 1148385 3 fpizlo 2015-12-10 18:44:29 -0800 (In reply to comment #2) > Comment on attachment 267111 [details] > the patch > > I would have guessed 2. Depends on the architecture and a lot of other things. On Intel, any address expression takes 1 cycle, regardless of its complexity. So if you do this: mov (%rax), %rax then you will spend 1 cycle deducing that the address is simply the thing in %rax. And if you do this: mov 42(%rcx,%rdx,4), %rax then you will also spend 1 cycle deducing what the address is. Therefore, if you had to pick between the following two snippets, you would pick the one with fewer instructions even though it computes the same address twice: This is faster: mov 42(%rcx,%rdx,4), %rax mov 46(%rcx,%rdx,4), %rdi than this: lea 42(%rcx,%rdx,4), %rsi mov (%rsi), %rax mov 4(%rsi), %rdi This would still be true even if the address expressions were identical rather than the second one being offset by +4. The first version is faster because even though we recompute the same seemingly complex address expression, it's actually free to do so. Hence, setting the threshold to 2 is probably not a good thing. It's far too low. The last time I wrote a compiler, I set the threshold to +Inf. Later on, I learned through whispers in the wind that you want to have some kind of upper limit, though I never learned the reason. I suspect that the reason is just registers. It's possible that in the "faster" example above, keeping %rcx and %rdx alive causes too much register pressure. You don't know if that's an issue at the time that you do instruction selection, and usually it won't be an issue. But you can see how if you really had a lot of uses of the same address, then the second form may be better because it requires only one register to be alive for the memory access to compute the address. So, 2 is probably too low because it adds instructions without reducing the amount of work, but +Inf is probably too high because at some point the register pressure of keeping all of the inputs to the address computation alive is a bigger issue than the cost of the "lea" instruction. 1148407 4 fpizlo 2015-12-10 19:42:31 -0800 Landed in http://trac.webkit.org/changeset/193941 267111 2015-12-10 09:16:24 -0800 2015-12-10 15:34:51 -0800 the patch blah.patch text/plain 2547 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTkzOTA5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBA CisyMDE1LTEyLTEwICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg Q29uc2lkZXIgc3RpbGwgbWF0Y2hpbmcgYW4gYWRkcmVzcyBleHByZXNzaW9uIGV2ZW4gaWYgQjMg aGFzIGFscmVhZHkgYXNzaWduZWQgYSBUbXAgdG8gaXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MDc3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIG5lZWQgc29tZSBoZXVyaXN0aWMgZm9yIHdoZW4g YW4gYWRkcmVzcyBzaG91bGQgYmUgY29tcHV0ZWQgYXMgYSBzZXBhcmF0ZSBpbnN0cnVjdGlvbi4g SXQncworICAgICAgICB1c3VhbGx5IHByb2ZpdGFibGUgdG8gc2luayB0aGUgYWRkcmVzcyBpbnRv IHRoZSBtZW1vcnkgYWNjZXNzLiBUaGUgcHJldmlvdXMgaGV1cmlzdGljIG1lYW50IHRoYXQKKyAg ICAgICAgdGhlIGFkZHJlc3Mgd291bGQgZ2V0IHNlcGFyYXRlIGluc3RydWN0aW9ucyBpZiBpdCB3 YXMgaW4gYSBzZXBhcmF0ZSBibG9jayBmcm9tIHRoZSBtZW1vcnkgYWNjZXNzLgorICAgICAgICBU aGlzIHdhcyBtZXNzaW5nIHVwIGNvZGVnZW4gb2YgdGhpbmdzIGxpa2UgUHV0QnlWYWwgb3V0LW9m LWJvdW5kcywgd2hlcmUgdGhlIGFkZHJlc3MgaXMgY29tcHV0ZWQKKyAgICAgICAgaW4gb25lIGJs b2NrIGFuZCB0aGVuIHVzZWQgaW4gYW5vdGhlci4gSSBkb24ndCB0aGluayB0aGF0IHdoaWNoIGJs b2NrIG93bnMgdGhlIGFkZHJlc3MKKyAgICAgICAgY29tcHV0YXRpb24gc2hvdWxkIGZhY3RvciBp bnRvIGFueSBoZXVyaXN0aWMgaGVyZSwgc2luY2UgaXQncyBzbyBmcmFnaWxlOiB0aGUgY29tcGls ZXIgbWF5IGxvd2VyCisgICAgICAgIHNvbWV0aGluZyBieSBzcGxpdHRpbmcgYmxvY2tzIGFuZCB3 ZSBkb24ndCB3YW50IHRoaXMgdG8gcnVpbiBwZXJmb3JtYW5jZS4KKworICAgICAgICBTbywgdGhp cyByZXBsYWNlcyB0aGF0IGhldXJpc3RpYyB3aXRoIGEgbW9yZSBzZW5zaWJsZSBvbmU6IHRoZSBh ZGRyZXNzIGNvbXB1dGF0aW9uIGdldHMgaXRzIG93bgorICAgICAgICBpbnN0cnVjdGlvbiBpZiBp dCBoYXMgYSBsb3Qgb2YgdXNlcy4gSW4gcHJhY3RpY2UgdGhpcyBtZWFucyB0aGF0IHdlIGFsd2F5 cyBzaW5rIHRoZSBhZGRyZXNzCisgICAgICAgIGNvbXB1dGF0aW9uIGludG8gdGhlIG1lbW9yeSBh Y2Nlc3MuCisKKyAgICAgICAgKiBiMy9CM0xvd2VyVG9BaXIuY3BwOgorICAgICAgICAoSlNDOjpC Mzo6QWlyOjpMb3dlclRvQWlyOjplZmZlY3RpdmVBZGRyKToKKwogMjAxNS0xMi0xMCAgQ3NhYmEg T3N6dHJvZ29uw6FjICA8b3NzeUB3ZWJraXQub3JnPgogCiAgICAgICAgIFtCM10gVXNlIG1hcmsg cHJhZ21hcyBvbmx5IGlmIGl0IGlzIHN1cHBvcnRlZApJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL2IzL0IzTG93ZXJUb0Fpci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRD b3JlL2IzL0IzTG93ZXJUb0Fpci5jcHAJKHJldmlzaW9uIDE5Mzg4OSkKKysrIFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9iMy9CM0xvd2VyVG9BaXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNjksOSAr MzY5LDEwIEBAIHByaXZhdGU6CiAgICAgLy8gVGhpcyB0dXJucyB0aGUgZ2l2ZW4gb3BlcmFuZCBp bnRvIGFuIGFkZHJlc3MuCiAgICAgQXJnIGVmZmVjdGl2ZUFkZHIoVmFsdWUqIGFkZHJlc3MpCiAg ICAgewotICAgICAgICAvLyBGSVhNRTogQ29uc2lkZXIgbWF0Y2hpbmcgYW4gYWRkcmVzcyBleHBy ZXNzaW9uIGV2ZW4gaWYgd2UndmUgYWxyZWFkeSBhc3NpZ25lZCBhCi0gICAgICAgIC8vIFRtcCB0 byBpdC4gaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MDc3NwotICAg ICAgICBpZiAobV92YWx1ZVRvVG1wW2FkZHJlc3NdKQorICAgICAgICBzdGF0aWMgY29uc3QgdW5z aWduZWQgbG90c09mVXNlcyA9IDEwOyAvLyBUaGlzIGlzIGFyYml0cmFyeSBhbmQgd2Ugc2hvdWxk IHR1bmUgaXQgZXZlbnR1YWxseS4KKyAgICAgICAgCisgICAgICAgIC8vIE9ubHkgbWF0Y2ggaWYg dGhlIGFkZHJlc3MgdmFsdWUgaXNuJ3QgdXNlZCBpbiBzb21lIGxhcmdlIG51bWJlciBvZiBwbGFj ZXMuCisgICAgICAgIGlmIChtX3VzZUNvdW50cy5udW1Vc2VzKGFkZHJlc3MpID4gbG90c09mVXNl cykKICAgICAgICAgICAgIHJldHVybiBBcmc6OmFkZHIodG1wKGFkZHJlc3MpKTsKICAgICAgICAg CiAgICAgICAgIHN3aXRjaCAoYWRkcmVzcy0+b3Bjb2RlKCkpIHsK