-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build with Clang + MSVC #1607
Build with Clang + MSVC #1607
Comments
I am trying to make Enzyme compatible with Clang + MSVC. There are some stubborn linker errors. It fails to link the LLVM Support library. Does this look familiar to anyone? Linker error
Steps to reproduce
|
Looks like unfortunately no one is too familiar with this issue. Do you want to join the next Enzyme meeting, so we can all try to debug this together? It would be Wednesday, 16:00 ET on Zoom: https://mit.zoom.us/j/96000853439 |
@ZuseZ4 Sure, it would be my pleasure. Thank you for the invite. |
Building a clang that can accept enzyme on windows with MSVC is tricky because not everything that enzyme needs from clang/opt is exported in the export table. |
I won't be much help until maybe tomorrow, but after a very cursory glance, you'll have to link against My apologies for the generic answer. I will be back in less than 24 hours with a (hopefully) better explanation and (again, hopefully), a solution – if no one else has solved it by then. I'm updating my debug build of Clang+LLVM to 17.0.6. Once that build finishes, I'll clone the repo and take a much closer look. In the meantime, I'll look through the |
Okay, I think I see what's happening. I'm still building LLVM at the moment, but after taking a closer look at Enzyme/enzyme/Enzyme/CMakeLists.txt Lines 53 to 82 in 11cc0f1
If you're linking against It should be as simple as adding llvm_map_components_to_libnames(llvmLibs Support)
target_link_libraries(LLVMEnzyme-${LLVM_VERSION_MAJOR} PRIVATE ${llvmLibs}) If that doesn't work, try manually linking without having LLVM "help" you: target_link_libraries(LLVMEnzyme-${LLVM_VERSION_MAJOR}
PRIVATE LLVMSupport)
# Or:
target_link_libraries(LLVMEnzyme-${LLVM_VERSION_MAJOR}
PRIVATE LLVMSupport.lib) I haven't looked at the Enzyme source code, but if it exposes anything from the support library, you'll have to do I gave a talk on building LLVM plugins for Windows under Visual Studio. You may not be building directly with MSVC, but even so, you may run into other problems getting If you do run into problems, you may have to change the function you use for defining your library targets -- specifically for Windows + MSVC. |
@jvstech Thank you for looking at this issue. The linker errors persist with your suggestions, but your insights are still informative. I found your talk especially helpful and well presented. Your demo repo will also be a great resource. We investigated In your presentation you discuss many tricky environmental settings that must be configured correctly. It is plausible that one such issue is causing the linker error. If your environment is working already, it would be a great help if you could attempt to build Enzyme with Clang + MSVC. Though, you have done a great deal already and have given us plenty to work with. |
@OwenTrokeBillard Not a problem. Happy to help in any way I can. I wasn't sure if It took a while (I had to re-configure LLVM with
That said, once I got past that missing symbol, there were several other libraries that that had to be added. Additionally, there were symbols the linker insisted were missing (from
It's very clearly listed with its defined section and with external linkage. The only thing I can think of is In the meantime, I'm going to try adding this function to the function(add_enzyme_library targetName)
cmake_parse_arguments(ELIB
""
""
"LINK_COMPONENTS"
${ARGN})
if ((WIN32 OR MSVC) AND NOT CYGWIN AND NOT MINGW)
add_library(${targetName} SHARED ${ELIB_UNPARSED_ARGUMENTS})
if (ELIB_LINK_COMPONENTS)
llvm_map_components_to_libnames(ENZYME_WINDOWS_LLVM_LIBS ${ELIB_LINK_COMPONENTS})
endif()
if (LLVM_LINK_COMPONENTS)
list(APPEND ELIB_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS})
endif()
if (ELIB_LINK_COMPONENTS)
target_link_libraries(${targetName} PUBLIC ${ENZYME_WINDOWS_LLVM_LIBS})
endif()
else()
add_llvm_library(${targetName} MODULE ${ELIB_UNPARSED_ARGUMENTS})
endif()
endfunction() I'll let you know if I get any further. |
By the way, you may be interested in this draft pull request with a prototype for building LLVM as DLLs on Windows. It's not done yet, but there is effort being made to get there. |
AH HA! I figured it out! Enzyme/enzyme/Enzyme/Enzyme.cpp Lines 28 to 36 in b6bb8d8
And, specifically: Enzyme/enzyme/Enzyme/Enzyme.cpp Line 29 in b6bb8d8
Do not do this. It's forbidden by the C++ standard because those access specifiers are part of the ABI. I know that's what the The giveaway was running
However, with verbose linker output, we can also see this:
The only difference is that the found symbol is I've removed the |
Thank you a lot for helping with this, having MSCV support will save us from workarounds on the Rust side. @wsmoses Do you remember why this line was added? |
It is needed for older LLVMs to extend scalar evolution with support for
must exit semantics. This was integrated into LLVM, but versions before the
import require it.
We can #if llvmversjon gate the define. For the rust folks that should be
fine since your LLVM is recent regardless and it isn’t needed.
…On Sat, Jan 20, 2024 at 8:21 PM Manuel Drehwald ***@***.***> wrote:
Thank you a lot for helping with this, having MSCV support will save us
from extra effort on the Rust side.
@wsmoses <https://github.com/wsmoses> Do you remember why this line was
added?
—
Reply to this email directly, view it on GitHub
<#1607 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXHQYZOTWPR5IT373VDYPRURXAVCNFSM6AAAAABBTZO4M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGQ3DSNZZHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Or actually looking at it, maybe not since the integration clearly is gated
on llvm 16 and adding it on the other side. What fails to build when
removing it?
…On Sat, Jan 20, 2024 at 8:39 PM Billy Moses ***@***.***> wrote:
It is needed for older LLVMs to extend scalar evolution with support for
must exit semantics. This was integrated into LLVM, but versions before the
import require it.
We can #if llvmversjon gate the define. For the rust folks that should be
fine since your LLVM is recent regardless and it isn’t needed.
On Sat, Jan 20, 2024 at 8:21 PM Manuel Drehwald ***@***.***>
wrote:
> Thank you a lot for helping with this, having MSCV support will save us
> from extra effort on the Rust side.
>
> @wsmoses <https://github.com/wsmoses> Do you remember why this line was
> added?
>
> —
> Reply to this email directly, view it on GitHub
> <#1607 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAJTUXHQYZOTWPR5IT373VDYPRURXAVCNFSM6AAAAABBTZO4M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGQ3DSNZZHE>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
Looks like the problem is that the exit limit type in scalar evolution is
marked private in LLVM but used by us.
This should hopefully be a simple patch to llvm upstream.
2024-01-21T01:54:09.3771619Z
/home/runner/work/Enzyme/Enzyme/enzyme/Enzyme/MustExitScalarEvolution.h:63:34:
error: ‘using ExitLimitCacheTy = class
llvm::ScalarEvolution::ExitLimitCache’ is private within this context
2024-01-21T01:54:09.3774131Z
…On Sat, Jan 20, 2024 at 8:41 PM Billy Moses ***@***.***> wrote:
Or actually looking at it, maybe not since the integration clearly is
gated on llvm 16 and adding it on the other side. What fails to build when
removing it?
On Sat, Jan 20, 2024 at 8:39 PM Billy Moses ***@***.***>
wrote:
> It is needed for older LLVMs to extend scalar evolution with support for
> must exit semantics. This was integrated into LLVM, but versions before the
> import require it.
>
> We can #if llvmversjon gate the define. For the rust folks that should be
> fine since your LLVM is recent regardless and it isn’t needed.
>
> On Sat, Jan 20, 2024 at 8:21 PM Manuel Drehwald ***@***.***>
> wrote:
>
>> Thank you a lot for helping with this, having MSCV support will save us
>> from extra effort on the Rust side.
>>
>> @wsmoses <https://github.com/wsmoses> Do you remember why this line was
>> added?
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#1607 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AAJTUXHQYZOTWPR5IT373VDYPRURXAVCNFSM6AAAAABBTZO4M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGQ3DSNZZHE>
>> .
>> You are receiving this because you were mentioned.Message ID:
>> ***@***.***>
>>
>
|
@jvstech Thank you for thoroughly investigating this. I never would have suspected redefining access specifiers was the culprit. You have saved me weeks of toiling. |
Just building a patched LLVM+Enzyme to make sure there are no other issues. |
There are more issues than just that. There are several direct uses of private member variables and functions.
and so on. This is one of those situations where I would make a copy of the required source and mark those members It's not perfect, but it's a sterile bandage for the time being. |
Not a problem. It's been a long weekend and I've had some spare cycles, so I'm happy to help. |
Oh darn, yeah I think we'd just want to replace all private linkages with protected there, which also hopefully wouldn't be that bad. However the long term solution is that MustExitScalarEvolution should just be upstreamed. |
I'm think I fixed this but could someone with a windows system test this out to confirm? I'm testing on fedora with
|
@skewballfox I personally don’t have access to a windows machine. However GitHub actions offer windows runners. |
@jedbrown iirc you had a windows machine, if you have some time, could you verify this builds please? Let's try without Rust for now, that should be easy to add later. |
cc @matinraayai who was starting to look at some of the linkage upstreaming |
@skewballfox once the linkages are fixed in upstream LLVM, the correct fix to Enzyme is to remove the |
is there an issue I can look at to get a bit more context? sorry new to contributing to llvm/enzyme |
|
No there isn't an issue (besides this one), but essentially your LLVM commit needs to be merged upstream into LLVM. Supposing your fix exists in LLVM, the correct thing to do in Enzyme is to remove these defines ( Enzyme/enzyme/Enzyme/Enzyme.cpp Line 29 in 26c4337
|
I submitted a PR with the one line change to LLVM. I'll update you if the status changes |
Hey, anyone know of anything projects downstream of enzyme making use of |
So I started looking throught the commit history and associated issues for Alternatively, anyone have ideas, suggestions, or links that could be useful for writing test for MESE's code? |
|
I'll try to take a look in a few days, but as you saw we didn't have any explicit unit tests for the new functionality so you're going to need to write them by hand |
Probably that involves forking the existing SE tests that I referenced in the LLVM PR, adding a flag to parse the must exit functionality, and then testing the new functionality behaves as expected |
It would be beneficial for Enzyme to build with Clang + MSVC on Windows. One use case is Rust integration. See EnzymeAD/rust#54.
Enzyme is presently cross-compiled for Windows with Clang + MinGW. With any luck, the only change required is updating
CMakeLists.txt
for Clang + MSVC compatibility.The text was updated successfully, but these errors were encountered: