Bug 138523 – [JSC] Lower switch statements at LLVM IR level when using FastISel.

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Juergen Ributzka]

[JSC] Lower switch statements at LLVM IR level when using FastISel.

[Juergen Ributzka]

Created attachment 241210 [details]
Patch

[Geoffrey Garen]

Comment on attachment 241210 [details]
Patch

r=me

Is there any downside to lowering early? Does this prevent lookup-table-based switch?

[Geoffrey Garen]

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

[Juergen Ributzka]

(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.

[Juergen Ributzka]

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

[Filip Pizlo]

Comment on attachment 241210 [details]
Patch

R=me if it builds. ;-)

[Juergen Ributzka]

(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.

[Geoffrey Garen]

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.

[Filip Pizlo]

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