138523 2014-11-07 15:04:19 -0800 [JSC] Lower switch statements at LLVM IR level when using FastISel. 2015-04-07 11:23:54 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 143489 P2 Normal --- 1 juergen webkit-unassigned fpizlo oldest_to_newest 1047193 0 juergen 2014-11-07 15:04:19 -0800 [JSC] Lower switch statements at LLVM IR level when using FastISel. 1047198 1 241210 juergen 2014-11-07 15:06:49 -0800 Created attachment 241210 Patch 1047232 2 241210 ggaren 2014-11-07 15:55:56 -0800 Comment on attachment 241210 Patch r=me Is there any downside to lowering early? Does this prevent lookup-table-based switch? 1047233 3 241210 ggaren 2014-11-07 15:56:43 -0800 Comment on attachment 241210 Patch ....Hmmm... looks like a missing #include?: /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:103:32: error: use of undeclared identifier 'LLVMAddLowerSwitchPass'; did you mean 'LLVMAddLoopUnswitchPass'? FOR_EACH_LLVM_API_FUNCTION(LLVM_API_FUNCTION_ASSIGNMENT); ^ In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:30: In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPI.h:31: /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:636:5: note: expanded from macro 'FOR_EACH_LLVM_API_FUNCTION' macro(void, AddLowerSwitchPass, (LLVMPassManagerRef PM)) \ ^ /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:102:20: note: expanded from macro 'LLVM_API_FUNCTION_ASSIGNMENT' result->name = LLVM##name; ^ <scratch space>:10:1: note: expanded from here LLVMAddLowerSwitchPass ^ In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:30: In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPI.h:31: In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:29: In file included from /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMHeaders.h:56: /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/LLVMForJavaScriptCore/include/llvm-c/Transforms/Scalar.h:78:6: note: 'LLVMAddLoopUnswitchPass' declared here 1047234 4 juergen 2014-11-07 16:01:35 -0800 (In reply to comment #2) > Comment on attachment 241210 [details] > Patch > > r=me > > Is there any downside to lowering early? Does this prevent > lookup-table-based switch? Yes, it prevents lookup-table-based switches, but FastISel isn't supporting them anyways. I haven't seen a performance regression yet, but we can revisit this if it should become a problem. The important thing here is that you don't want to fall-back to SelectionDAG, which would increase compile time. 1047235 5 juergen 2014-11-07 16:03:25 -0800 It works on my machines with the latest headers. I guess the llvm src code is not up-to-date on that machine. From where do you get the code - from the LLVM drops? (In reply to comment #3) > Comment on attachment 241210 [details] > Patch > > ....Hmmm... looks like a missing #include?: > > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: > 103:32: error: use of undeclared identifier 'LLVMAddLowerSwitchPass'; did > you mean 'LLVMAddLoopUnswitchPass'? > FOR_EACH_LLVM_API_FUNCTION(LLVM_API_FUNCTION_ASSIGNMENT); > ^ > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: > 30: > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPI.h:31: > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:636:5: > note: expanded from macro 'FOR_EACH_LLVM_API_FUNCTION' > macro(void, AddLowerSwitchPass, (LLVMPassManagerRef PM)) \ > ^ > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: > 102:20: note: expanded from macro 'LLVM_API_FUNCTION_ASSIGNMENT' > result->name = LLVM##name; > ^ > <scratch space>:10:1: note: expanded from here > LLVMAddLowerSwitchPass > ^ > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: > 30: > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPI.h:31: > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:29: > In file included from > /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/llvm/LLVMHeaders.h:56: > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/LLVMForJavaScriptCore/ > include/llvm-c/Transforms/Scalar.h:78:6: note: 'LLVMAddLoopUnswitchPass' > declared here 1048467 6 241210 fpizlo 2014-11-13 13:17:32 -0800 Comment on attachment 241210 Patch R=me if it builds. ;-) 1048469 7 juergen 2014-11-13 13:20:25 -0800 (In reply to comment #6) > Comment on attachment 241210 [details] > Patch > > R=me if it builds. ;-) It does build with LLVM trunk. The C API for the switch lowering pass was added after the LLVM drops where made, so that is why this is failing on the buildbots. 1074154 8 241210 ggaren 2015-03-04 11:52:17 -0800 Comment on attachment 241210 Patch Let's come up with a way to adopt this API conditionally that doesn't break the build or harm performance when using older versions of LLVM. 1083358 9 fpizlo 2015-04-07 11:23:54 -0700 *** This bug has been marked as a duplicate of bug 143489 *** 241210 2014-11-07 15:06:49 -0800 2015-03-04 11:52:17 -0800 Patch bug-138523-20141107150659.patch text/plain 3234 juergen U3VidmVyc2lvbiBSZXZpc2lvbjogMTc1NzY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA3 NmMzNmVkMDZhNWYzMDMwMThlZDI1NjRlN2ZkZGJiOWIwYzc2YWRjLi45MmQ3MmQwYjQzOTU0MjA3 OTUwNmI1NjhjNjY2MDg4OGVjMjUxZDUwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs NSArMSwyMCBAQAogMjAxNC0xMS0wNyAgSnVlcmdlbiBSaWJ1dHprYSAgPGp1ZXJnZW5AYXBwbGUu Y29tPgogCisgICAgICAgIFtKU0NdIExvd2VyIHN3aXRjaCBzdGF0ZW1lbnRzIGF0IExMVk0gSVIg bGV2ZWwgd2hlbiB1c2luZyBGYXN0SVNlbC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEzODUyMworICAgICAgICAKKyAgICAgICAgTExWTSBGYXN0SVNl bCBkb2Vzbid0IHN1cHBvcnQgc3dpdGNoIHN0YXRlbWVudCBsb3dlcmluZyBhbmQgZmFsbHMgYmFj aworICAgICAgICB0byBTZWxlY3Rpb25EQUcuIFdlIHdvdWxkIGxpa2UgdG8gYXZvaWQgdGhpcyBj b21waWxlIHRpbWUgcGVuYWx0eSBhbmQKKyAgICAgICAgaW5zdGVhZCBsb3dlciB0aGUgc3dpdGNo IHN0YXRlbWVudCBiZWZvcmUgaW5zdHJ1Y3Rpb24gc2VsZWN0aW9uLgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZnRsL0ZUTENvbXBpbGUuY3BwOgor ICAgICAgICAoSlNDOjpGVEw6Om1tQWxsb2NhdGVEYXRhU2VjdGlvbik6CisgICAgICAgICogbGx2 bS9MTFZNQVBJRnVuY3Rpb25zLmg6CisKKzIwMTQtMTEtMDcgIEp1ZXJnZW4gUmlidXR6a2EgIDxq dWVyZ2VuQGFwcGxlLmNvbT4KKwogICAgICAgICBVcGRhdGUgV2ViS2l0IHRvIGJ1aWxkIHdpdGgg TExWTSBUT1QKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTEzODUxOQogCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTENvbXBp bGUuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExDb21waWxlLmNwcAppbmRleCAy NmE3YmM1ZWQ3NjdlZGY1NjczNjcxZDAyMzUzNTYyMjNiZDYzZWNhLi4xYjA1MDYxNWFhNzlkZjIy NDI1ZTNiZjAwYmYwNmZlYTc3NzVjODk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvZnRsL0ZUTENvbXBpbGUuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9mdGwvRlRM Q29tcGlsZS5jcHAKQEAgLTY1OSw3ICs2NTksMTEgQEAgdm9pZCBjb21waWxlKFN0YXRlJiBzdGF0 ZSwgU2FmZXBvaW50OjpSZXN1bHQmIHNhZmVwb2ludFJlc3VsdCkKICAgICAgICAgICAgIGxsdm0t PkFkZEdWTlBhc3MobW9kdWxlUGFzc2VzKTsKICAgICAgICAgICAgIGxsdm0tPkFkZENGR1NpbXBs aWZpY2F0aW9uUGFzcyhtb2R1bGVQYXNzZXMpOwogICAgICAgICAgICAgbGx2bS0+QWRkRGVhZFN0 b3JlRWxpbWluYXRpb25QYXNzKG1vZHVsZVBhc3Nlcyk7Ci0gCisgICAgICAgICAgICAKKyAgICAg ICAgICAgIC8vIEluIEZhc3RJU2VsIG1vZGUgbG93ZXIgc3dpdGNoIHN0YXRlbWVudHMgYXQgSVIg bGV2ZWwuCisgICAgICAgICAgICBpZiAoT3B0aW9uczo6ZW5hYmxlTExWTUZhc3RJU2VsKCkpCisg ICAgICAgICAgICAgICAgbGx2bS0+QWRkTG93ZXJTd2l0Y2hQYXNzKG1vZHVsZVBhc3Nlcyk7Cisg ICAgICAgICAgICAKICAgICAgICAgICAgIGxsdm0tPlJ1blBhc3NNYW5hZ2VyKG1vZHVsZVBhc3Nl cywgc3RhdGUubW9kdWxlKTsKICAgICAgICAgfSBlbHNlIHsKICAgICAgICAgICAgIExMVk1QYXNz TWFuYWdlckJ1aWxkZXJSZWYgcGFzc0J1aWxkZXIgPSBsbHZtLT5QYXNzTWFuYWdlckJ1aWxkZXJD cmVhdGUoKTsKQEAgLTY4Miw2ICs2ODYsMTAgQEAgdm9pZCBjb21waWxlKFN0YXRlJiBzdGF0ZSwg U2FmZXBvaW50OjpSZXN1bHQmIHNhZmVwb2ludFJlc3VsdCkKICAgICAgICAgICAgICAgICBsbHZt LT5SdW5GdW5jdGlvblBhc3NNYW5hZ2VyKGZ1bmN0aW9uUGFzc2VzLCBmdW5jdGlvbik7CiAgICAg ICAgICAgICBsbHZtLT5GaW5hbGl6ZUZ1bmN0aW9uUGFzc01hbmFnZXIoZnVuY3Rpb25QYXNzZXMp OwogICAgICAgICAKKyAgICAgICAgICAgIC8vIEluIEZhc3RJU2VsIG1vZGUgbG93ZXIgc3dpdGNo IHN0YXRlbWVudHMgYXQgSVIgbGV2ZWwuCisgICAgICAgICAgICBpZiAoT3B0aW9uczo6ZW5hYmxl TExWTUZhc3RJU2VsKCkpCisgICAgICAgICAgICAgICAgbGx2bS0+QWRkTG93ZXJTd2l0Y2hQYXNz KG1vZHVsZVBhc3Nlcyk7CisgICAgICAgICAgICAKICAgICAgICAgICAgIGxsdm0tPlJ1blBhc3NN YW5hZ2VyKG1vZHVsZVBhc3Nlcywgc3RhdGUubW9kdWxlKTsKICAgICAgICAgfQogCmRpZmYgLS1n aXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvbGx2bS9MTFZNQVBJRnVuY3Rpb25zLmggYi9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvbGx2bS9MTFZNQVBJRnVuY3Rpb25zLmgKaW5kZXggMTRjNGYwMDBl NmJiZjNhM2FkN2I1ODNmNzIwMDUxM2ZhNmQ3Y2EyNi4uN2RkODNkYzgyYzZjMWYzNGU3OWMzZmQ1 NjRkY2IyMDQ4YWJmZWJiNSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2xsdm0v TExWTUFQSUZ1bmN0aW9ucy5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbHZtL0xMVk1B UElGdW5jdGlvbnMuaApAQCAtNjI4LDYgKzYyOCw3IEBACiAgICAgbWFjcm8odm9pZCwgQWRkTG9v cFVuc3dpdGNoUGFzcywgKExMVk1QYXNzTWFuYWdlclJlZiBQTSkpIFwKICAgICBtYWNybyh2b2lk LCBBZGRNZW1DcHlPcHRQYXNzLCAoTExWTVBhc3NNYW5hZ2VyUmVmIFBNKSkgXAogICAgIG1hY3Jv KHZvaWQsIEFkZFBhcnRpYWxseUlubGluZUxpYkNhbGxzUGFzcywgKExMVk1QYXNzTWFuYWdlclJl ZiBQTSkpIFwKKyAgICBtYWNybyh2b2lkLCBBZGRMb3dlclN3aXRjaFBhc3MsIChMTFZNUGFzc01h bmFnZXJSZWYgUE0pKSBcCiAgICAgbWFjcm8odm9pZCwgQWRkUHJvbW90ZU1lbW9yeVRvUmVnaXN0 ZXJQYXNzLCAoTExWTVBhc3NNYW5hZ2VyUmVmIFBNKSkgXAogICAgIG1hY3JvKHZvaWQsIEFkZFJl YXNzb2NpYXRlUGFzcywgKExMVk1QYXNzTWFuYWdlclJlZiBQTSkpIFwKICAgICBtYWNybyh2b2lk LCBBZGRTQ0NQUGFzcywgKExMVk1QYXNzTWFuYWdlclJlZiBQTSkpIFwK