Bug 138523

Summary: [JSC] Lower switch statements at LLVM IR level when using FastISel.
Product: WebKit Reporter: Juergen Ributzka <juergen>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: fpizlo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ggaren: review-

Description Juergen Ributzka 2014-11-07 15:04:19 PST
[JSC] Lower switch statements at LLVM IR level when using FastISel.
Comment 1 Juergen Ributzka 2014-11-07 15:06:49 PST
Created attachment 241210 [details]
Patch
Comment 2 Geoffrey Garen 2014-11-07 15:55:56 PST
Comment on attachment 241210 [details]
Patch

r=me

Is there any downside to lowering early? Does this prevent lookup-table-based switch?
Comment 3 Geoffrey Garen 2014-11-07 15:56:43 PST
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
Comment 4 Juergen Ributzka 2014-11-07 16:01:35 PST
(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.
Comment 5 Juergen Ributzka 2014-11-07 16:03:25 PST
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
Comment 6 Filip Pizlo 2014-11-13 13:17:32 PST
Comment on attachment 241210 [details]
Patch

R=me if it builds. ;-)
Comment 7 Juergen Ributzka 2014-11-13 13:20:25 PST
(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.
Comment 8 Geoffrey Garen 2015-03-04 11:52:17 PST
Comment on attachment 241210 [details]
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.
Comment 9 Filip Pizlo 2015-04-07 11:23:54 PDT

*** This bug has been marked as a duplicate of bug 143489 ***