104377 2012-12-07 09:57:37 -0800 [sh4] Implement add64 for SH4 assembler after r136601 2012-12-11 04:16:38 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Linux RESOLVED FIXED P2 Normal --- 0 jbriance webkit-unassigned barraclough fpizlo msaboff oliver ossy webkit.review.bot zherczeg oldest_to_newest 785946 0 jbriance 2012-12-07 09:57:37 -0800 /local/wkit/slavebuildbot/workspace/qt-linux-sh4-release/build/Source/JavaScriptCore/jit/JIT.cpp: In member function ‘void JSC::JIT::privateCompileMainPass()’: /local/wkit/slavebuildbot/workspace/qt-linux-sh4-release/build/Source/JavaScriptCore/jit/JIT.cpp:243:80: error: ‘add64’ was not declared in this scope 787068 1 178484 jbriance 2012-12-10 01:13:30 -0800 Created attachment 178484 Add add64 implementation in SH4 JIT 788256 2 178484 zherczeg 2012-12-11 03:06:49 -0800 Comment on attachment 178484 Add add64 implementation in SH4 JIT Looks good, few comments: View in context: https://bugs.webkit.org/attachment.cgi?id=178484&action=review > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:410 > + // add 32-bit LSB first WebKit comment style is different. Please use full sentences. > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:411 > + m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address In WebKit you don't need to explain trivial facts :) Too much comments makes the code less readable. See http://www.webkit.org/coding/coding-style.html comments section. > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:416 > + m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address I am not sure how SH4 works, but if this requires a memory load, a third scratch register could be used to store the address of int64. I don't know you have a third scratch, so this might be unavoidable. 788265 3 178484 jbriance 2012-12-11 03:18:17 -0800 Comment on attachment 178484 Add add64 implementation in SH4 JIT View in context: https://bugs.webkit.org/attachment.cgi?id=178484&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:411 >> + m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address > > In WebKit you don't need to explain trivial facts :) Too much comments makes the code less readable. > See http://www.webkit.org/coding/coding-style.html comments section. Ok, I'll submit a new patch >> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:416 >> + m_assembler.loadConstant(reinterpret_cast<uint32_t>(address.m_ptr), scr1); // src1 = int64 address > > I am not sure how SH4 works, but if this requires a memory load, a third scratch register could be used to store the address of int64. I don't know you have a third scratch, so this might be unavoidable. Unfortunately, there are only 2 scratch registers for sh4 JIT yet (see claimScratch() implementation) 788273 4 178767 jbriance 2012-12-11 03:29:43 -0800 Created attachment 178767 Add add64 implementation in SH4 JIT (with comments update) 788275 5 178767 zherczeg 2012-12-11 03:34:03 -0800 Comment on attachment 178767 Add add64 implementation in SH4 JIT (with comments update) r=me. Nice patch. 788305 6 178767 webkit.review.bot 2012-12-11 04:16:35 -0800 Comment on attachment 178767 Add add64 implementation in SH4 JIT (with comments update) Clearing flags on attachment: 178767 Committed r137287: <http://trac.webkit.org/changeset/137287> 788307 7 webkit.review.bot 2012-12-11 04:16:38 -0800 All reviewed patches have been landed. Closing bug. 178484 2012-12-10 01:13:30 -0800 2012-12-11 03:30:33 -0800 Add add64 implementation in SH4 JIT javascript_sh4_jit_add64.patch text/plain 2705 jbriance SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTM3MTE5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDEyLTEyLTEwICBKdWxpZW4gQlJJQU5DRUFVICAgPGpicmlhbmNlYXVAbmRzLmNvbT4KKwor ICAgICAgICBJbXBsZW1lbnQgYWRkNjQgZm9yIFNINCBhc3NlbWJsZXIgdG8gZml4IGJ1aWxkIGFm dGVyIHIxMzY2MDEKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEwNDM3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyU0g0Lmg6CisgICAgICAgIChKU0M6Ok1hY3Jv QXNzZW1ibGVyU0g0OjphZGQ2NCk6CisgICAgICAgIChNYWNyb0Fzc2VtYmxlclNINCk6CisKIDIw MTItMTItMDkgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBERkcg QXJyYXlQdXNoL1BvcCBzaG91bGQgbm90IHBhc3MgdGhlaXIgc2Vjb25kIGNoaWxkIGFzIHRoZSBp bmRleCBmb3IgYmxlc3NBcnJheU9wZXJhdGlvbigpCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENv cmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyU0g0LmgKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0ph dmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlclNINC5oCShyZXZpc2lvbiAxMzcx MTgpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyU0g0 LmgJKHdvcmtpbmcgY29weSkKQEAgLTQwMiw2ICs0MDIsMzQgQEAgdm9pZCBvcjMyKFRydXN0ZWRJ bW0zMiBpbW0sIFJlZ2lzdGVySUQgcwogICAgICAgICByZWxlYXNlU2NyYXRjaChzY3JhdGNoUmVn KTsKICAgICB9CiAKKyAgICB2b2lkIGFkZDY0KFRydXN0ZWRJbW0zMiBpbW0sIEFic29sdXRlQWRk cmVzcyBhZGRyZXNzKQorICAgIHsKKyAgICAgICAgUmVnaXN0ZXJJRCBzY3IxID0gY2xhaW1TY3Jh dGNoKCk7CisgICAgICAgIFJlZ2lzdGVySUQgc2NyMiA9IGNsYWltU2NyYXRjaCgpOworCisgICAg ICAgIC8vIGFkZCAzMi1iaXQgTFNCIGZpcnN0CisgICAgICAgIG1fYXNzZW1ibGVyLmxvYWRDb25z dGFudChyZWludGVycHJldF9jYXN0PHVpbnQzMl90PihhZGRyZXNzLm1fcHRyKSwgc2NyMSk7IC8v IHNyYzEgPSBpbnQ2NCBhZGRyZXNzCisgICAgICAgIG1fYXNzZW1ibGVyLm1vdmxNZW1SZWcoc2Ny MSwgc2NyMSk7IC8vIHNjcjEgPSAzMi1iaXQgTFNCIG9mIGludDY0IEAgYWRkcmVzcworICAgICAg ICBtX2Fzc2VtYmxlci5sb2FkQ29uc3RhbnQoaW1tLm1fdmFsdWUsIHNjcjIpOyAvLyBzY3IyID0g aW1tIHZhbHVlCisgICAgICAgIG1fYXNzZW1ibGVyLmNscnQoKTsKKyAgICAgICAgbV9hc3NlbWJs ZXIuYWRkY2xSZWdSZWcoc2NyMSwgc2NyMik7IC8vIHNjcjIgPSBpbW0gKyAzMi1iaXQgTFNCIG9m IGludDY0IEAgYWRkcmVzcywgVD1jYXJyeQorICAgICAgICBtX2Fzc2VtYmxlci5sb2FkQ29uc3Rh bnQocmVpbnRlcnByZXRfY2FzdDx1aW50MzJfdD4oYWRkcmVzcy5tX3B0ciksIHNjcjEpOyAvLyBz cmMxID0gaW50NjQgYWRkcmVzcworICAgICAgICBtX2Fzc2VtYmxlci5tb3ZsUmVnTWVtKHNjcjIs IHNjcjEpOyAvLyB1cGRhdGUgYWRkcmVzcyB3aXRoIDMyLWJpdCBMU0IgcmVzdWx0CisKKyAgICAg ICAgLy8gdGhlbiBhZGQgMzItYml0IE1TQgorICAgICAgICBtX2Fzc2VtYmxlci5hZGRsSW1tOHIo NCwgc2NyMSk7IC8vIHNyYzEgPSBpbnQ2NCBhZGRyZXNzICsgNAorICAgICAgICBtX2Fzc2VtYmxl ci5tb3ZsTWVtUmVnKHNjcjEsIHNjcjEpOyAvLyBzY3IxID0gMzItYml0IE1TQiBvZiBpbnQ2NCBA IGFkZHJlc3MKKyAgICAgICAgbV9hc3NlbWJsZXIubW92dChzY3IyKTsgLy8gc2NyMiA9IG92ZXJm bG93L3VuZGVyZmxvdyBiaXQKKyAgICAgICAgaWYgKGltbS5tX3ZhbHVlIDwgMCkKKyAgICAgICAg ICAgIG1fYXNzZW1ibGVyLmFkZGxJbW04cigtMSwgc2NyMik7IC8vIHNpZ24gZXh0ZW5kIGltbSB2 YWx1ZSBpZiBuZWVkZWQKKyAgICAgICAgbV9hc3NlbWJsZXIuYWRkdmxSZWdSZWcoc2NyMiwgc2Ny MSk7IC8vIHNjcjEgPSBpbnQ2NCAzMi1iaXQgTVNCIHJlc3VsdAorICAgICAgICBtX2Fzc2VtYmxl ci5sb2FkQ29uc3RhbnQocmVpbnRlcnByZXRfY2FzdDx1aW50MzJfdD4oYWRkcmVzcy5tX3B0cikg KyA0LCBzY3IyKTsgLy8gc3JjMiA9IGludDY0IGFkZHJlc3MgKyA0CisgICAgICAgIG1fYXNzZW1i bGVyLm1vdmxSZWdNZW0oc2NyMSwgc2NyMik7IC8vIHVwZGF0ZSBhZGRyZXNzIHdpdGggMzItYml0 IE1TQiByZXN1bHQKKworICAgICAgICByZWxlYXNlU2NyYXRjaChzY3IyKTsKKyAgICAgICAgcmVs ZWFzZVNjcmF0Y2goc2NyMSk7CisgICAgfQorCiAgICAgdm9pZCBzdWIzMihUcnVzdGVkSW1tMzIg aW1tLCBSZWdpc3RlcklEIGRlc3QpCiAgICAgewogICAgICAgICBpZiAobV9hc3NlbWJsZXIuaXNJ bW1lZGlhdGUoLWltbS5tX3ZhbHVlKSkgewo= 178767 2012-12-11 03:29:43 -0800 2012-12-11 04:16:34 -0800 Add add64 implementation in SH4 JIT (with comments update) javascript_sh4_jit_add64_v2.patch text/plain 2451 jbriance SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTM3Mjc5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDEyLTEyLTExICBKdWxpZW4gQlJJQU5DRUFVICAgPGpicmlhbmNlYXVAbmRzLmNvbT4KKwor ICAgICAgICBJbXBsZW1lbnQgYWRkNjQgZm9yIFNINCBhc3NlbWJsZXIgdG8gZml4IGJ1aWxkIGFm dGVyIHIxMzY2MDEKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEwNDM3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyU0g0Lmg6CisgICAgICAgIChKU0M6Ok1hY3Jv QXNzZW1ibGVyU0g0OjphZGQ2NCk6CisgICAgICAgIChNYWNyb0Fzc2VtYmxlclNINCk6CisKIDIw MTItMTItMTAgIFl1cnkgU2VtaWtoYXRza3kgIDx5dXJ5c0BjaHJvbWl1bS5vcmc+CiAKICAgICAg ICAgTWVtb3J5IGluc3RydW1lbnRhdGlvbjogbWFrZSBzdXJlIGVhY2ggZWRnZSBpcyByZXBvcnRl ZCBvbmx5IG9uY2UKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9B c3NlbWJsZXJTSDQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1i bGVyL01hY3JvQXNzZW1ibGVyU0g0LmgJKHJldmlzaW9uIDEzNzI3NykKKysrIFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJTSDQuaAkod29ya2luZyBjb3B5KQpA QCAtNDAyLDYgKzQwMiwzNCBAQCB2b2lkIG9yMzIoVHJ1c3RlZEltbTMyIGltbSwgUmVnaXN0ZXJJ RCBzCiAgICAgICAgIHJlbGVhc2VTY3JhdGNoKHNjcmF0Y2hSZWcpOwogICAgIH0KIAorICAgIHZv aWQgYWRkNjQoVHJ1c3RlZEltbTMyIGltbSwgQWJzb2x1dGVBZGRyZXNzIGFkZHJlc3MpCisgICAg eworICAgICAgICBSZWdpc3RlcklEIHNjcjEgPSBjbGFpbVNjcmF0Y2goKTsKKyAgICAgICAgUmVn aXN0ZXJJRCBzY3IyID0gY2xhaW1TY3JhdGNoKCk7CisKKyAgICAgICAgLy8gQWRkIDMyLWJpdCBM U0IgZmlyc3QuCisgICAgICAgIG1fYXNzZW1ibGVyLmxvYWRDb25zdGFudChyZWludGVycHJldF9j YXN0PHVpbnQzMl90PihhZGRyZXNzLm1fcHRyKSwgc2NyMSk7CisgICAgICAgIG1fYXNzZW1ibGVy Lm1vdmxNZW1SZWcoc2NyMSwgc2NyMSk7IC8vIHNjcjEgPSAzMi1iaXQgTFNCIG9mIGludDY0IEAg YWRkcmVzcworICAgICAgICBtX2Fzc2VtYmxlci5sb2FkQ29uc3RhbnQoaW1tLm1fdmFsdWUsIHNj cjIpOworICAgICAgICBtX2Fzc2VtYmxlci5jbHJ0KCk7CisgICAgICAgIG1fYXNzZW1ibGVyLmFk ZGNsUmVnUmVnKHNjcjEsIHNjcjIpOworICAgICAgICBtX2Fzc2VtYmxlci5sb2FkQ29uc3RhbnQo cmVpbnRlcnByZXRfY2FzdDx1aW50MzJfdD4oYWRkcmVzcy5tX3B0ciksIHNjcjEpOworICAgICAg ICBtX2Fzc2VtYmxlci5tb3ZsUmVnTWVtKHNjcjIsIHNjcjEpOyAvLyBVcGRhdGUgYWRkcmVzcyB3 aXRoIDMyLWJpdCBMU0IgcmVzdWx0LgorCisgICAgICAgIC8vIFRoZW4gYWRkIDMyLWJpdCBNU0Iu CisgICAgICAgIG1fYXNzZW1ibGVyLmFkZGxJbW04cig0LCBzY3IxKTsKKyAgICAgICAgbV9hc3Nl bWJsZXIubW92bE1lbVJlZyhzY3IxLCBzY3IxKTsgLy8gc2NyMSA9IDMyLWJpdCBNU0Igb2YgaW50 NjQgQCBhZGRyZXNzCisgICAgICAgIG1fYXNzZW1ibGVyLm1vdnQoc2NyMik7CisgICAgICAgIGlm IChpbW0ubV92YWx1ZSA8IDApCisgICAgICAgICAgICBtX2Fzc2VtYmxlci5hZGRsSW1tOHIoLTEs IHNjcjIpOyAvLyBTaWduIGV4dGVuZCBpbW0gdmFsdWUgaWYgbmVlZGVkLgorICAgICAgICBtX2Fz c2VtYmxlci5hZGR2bFJlZ1JlZyhzY3IyLCBzY3IxKTsKKyAgICAgICAgbV9hc3NlbWJsZXIubG9h ZENvbnN0YW50KHJlaW50ZXJwcmV0X2Nhc3Q8dWludDMyX3Q+KGFkZHJlc3MubV9wdHIpICsgNCwg c2NyMik7CisgICAgICAgIG1fYXNzZW1ibGVyLm1vdmxSZWdNZW0oc2NyMSwgc2NyMik7IC8vIFVw ZGF0ZSAoYWRkcmVzcyArIDQpIHdpdGggMzItYml0IE1TQiByZXN1bHQuCisKKyAgICAgICAgcmVs ZWFzZVNjcmF0Y2goc2NyMik7CisgICAgICAgIHJlbGVhc2VTY3JhdGNoKHNjcjEpOworICAgIH0K KwogICAgIHZvaWQgc3ViMzIoVHJ1c3RlZEltbTMyIGltbSwgUmVnaXN0ZXJJRCBkZXN0KQogICAg IHsKICAgICAgICAgaWYgKG1fYXNzZW1ibGVyLmlzSW1tZWRpYXRlKC1pbW0ubV92YWx1ZSkpIHsK