Skip to content
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

Refactor Makefile and run.sh into build.jl script #1

Merged
merged 11 commits into from
Jan 5, 2024
Merged

Refactor Makefile and run.sh into build.jl script #1

merged 11 commits into from
Jan 5, 2024

Conversation

mofeing
Copy link

@mofeing mofeing commented Jan 2, 2024

With this code, now bindings are automatically generated on package build (which happens just when the package is instantiated).

It uses LLVM_full_jll to generate the bindings, so no custom LLVM installation is needed. The version is set to LLVM 15 to maximize compatibility with Julia's LLVM (although maybe this is not needed). Some dialects are deactivated due to errors.

The only custom thing that is needed currently is that LLVM_full_jll does not provide a libc++ build, so the -isysroot flag needs to be added (⚠️ currently set to my machine's configuration). Edit it for your machine.

@jumerckx
Copy link
Owner

jumerckx commented Jan 3, 2024

This is very cool, thanks a lot!
I'll try running it later today and would like to merge this quickly :)
If I understand correctly, the end goal would be to have a jll with the generator binary instead of having the user compile it from source?

@mofeing
Copy link
Author

mofeing commented Jan 3, 2024

This is very cool, thanks a lot! I'll try running it later today and would like to merge this quickly :)

Great!

If I understand correctly, the end goal would be to have a jll with the generator binary instead of having the user compile it from source?

Exactly. Also, this should work for different LLVM versions as @vchuravy asked (although we will need to add some switches to select the corresponding dialects for each version).

Something not solved is how we can select the correct LLVM_full_jll version for each Julia's LLVM version (e.g. Julia 1.10 uses LLVM 15, but Julia 1.11 will probably use LLVM 16), or even select a custom LLVM. As @vchuravy pointed, breaking changes might occur between LLVM versions so maybe we cannot just use the latest LLVM version.

I would merge this now as it improves over the current fragile toolchain, but would consider an alternative in the future.

@jumerckx
Copy link
Owner

jumerckx commented Jan 3, 2024

I tried building on my machine without success.
But I heavily suspect an error from my side.

extra = [
    "-lstdc++",
    "-rpath",
    joinpath(LLVM_full_jll.artifact_dir, "lib")
]
ERROR: Error building `MLIR`:
/usr/bin/x86_64-linux-gnu-ld: /tmp/jl-generators-61941d.o: in function `emitOpTableDefs(llvm::RecordKeeper const&, llvm::raw_ostream&)':
jl-generators.cc:(.text+0x45): undefined reference to `llvm::RecordKeeper::getAllDerivedDefinitionsIfDefined(llvm::StringRef) const'
/usr/bin/x86_64-linux-gnu-ld: jl-generators.cc:(.text+0x19e): undefined reference to `mlir::tblgen::Operator::Operator(llvm::Record const&)'
/usr/bin/x86_64-linux-gnu-ld: jl-generators.cc:(.text+0x247): undefined reference to `mlir::tblgen::Operator::getOperationName[abi:cxx11]() const'
/usr/bin/x86_64-linux-gnu-ld: jl-generators.cc:(.text+0x253): undefined reference to `mlir::tblgen::Operator::getDialectName() const'
/usr/bin/x86_64-linux-gnu-ld: jl-generators.cc:(.text+0x67f): undefined reference to `mlir::tblgen::Operator::hasDescription() const'
...
Environment
- llvm-config = /home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/tools/llvm-config
- clang = /home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/tools/clang
- CXXFLAGS = SubString{String}["-I/home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/include", "-std=c++14", "-fno-exceptions", "-fno-rtti", "-D_GNU_SOURCE", "-D__STDC_CONSTANT_MACROS", "-D__STDC_FORMAT_MACROS", "-D__STDC_LIMIT_MACROS"]
- LDFLAGS = SubString{String}["-L/home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/lib"]
- INCLUDE_PATH = /home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/include
- DIALECTS_PATH = /home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/include/mlir/Dialect
Compiling TableGen generator...
- extra flags = ["-lstdc++", "-rpath", "/home/jumerckx/.julia/artifacts/414276a44b10b52ca36eff2db0e9085c785fac9d/lib"]

This shouldn't necessarily block merging, though. Could you rebase on vc/tblgen?

@mofeing
Copy link
Author

mofeing commented Jan 3, 2024

It seems like you are not linking against libLLVM and libMLIR but it should, since the flags -lLLVM and -lMLIR are included in the libs variable... You haven't touched the libs variable right?

This shouldn't necessarily block merging, though. Could you rebase on vc/tblgen?

Do you mean your vc/tblgen or the one on upstream repository?

@jumerckx
Copy link
Owner

jumerckx commented Jan 4, 2024

It seems like you are not linking against libLLVM and libMLIR but it should, since the flags -lLLVM and -lMLIR are included in the libs variable... You haven't touched the libs variable right?

I have not, only extra was changed. But when I remove -lLLVM from libs I get undefined references for even more symbols which tells me the libraries must be getting picked up... I don't know how involved it would be to get build running on CI, maybe the problem doesn't occur there?

Do you mean your vc/tblgen or the one on upstream repository?

I was thinking on my fork would be easiest but I'm no git power user so there might be easier ways :)

@mofeing
Copy link
Author

mofeing commented Jan 4, 2024

I have not, only extra was changed. But when I remove -lLLVM from libs I get undefined references for even more symbols which tells me the libraries must be getting picked up... I don't know how involved it would be to get build running on CI, maybe the problem doesn't occur there?

Weird, gonna try in a Docker container.

I was thinking on my fork would be easiest but I'm no git power user so there might be easier ways :)

Rebased on top of your fork ;)

@jumerckx
Copy link
Owner

jumerckx commented Jan 4, 2024

Thank you!
Build works like a charm, the copying to src still throws an error for me because the project layout is slightly different.
ERROR: LoadError: SystemError: opening file "../src/Dialects/Dialects.jl": No such file or directory

@mofeing
Copy link
Author

mofeing commented Jan 4, 2024

I've replaced the path literal for joinpath(@__DIR__, "..", "src", "Dialects", "Dialects.jl"), so it's more portable. Try again.

@jumerckx
Copy link
Owner

jumerckx commented Jan 4, 2024

That doesn't fix it because the dialects directory is lowercased. This could be changed, I'm not sure what the proper naming convention is.

@mofeing
Copy link
Author

mofeing commented Jan 5, 2024

Weird, because in my machine is in uppercase. I've searched a lil bit and looks like Git is configured to be case insensitive. 🙃

I will change it now to dialects because that's the name when you clone the repo. I was trying to put it in uppercase because that was also the convention of the files inside of it.

src/Dialects.jl Outdated Show resolved Hide resolved
deps/build.jl Outdated Show resolved Hide resolved
@jumerckx
Copy link
Owner

jumerckx commented Jan 5, 2024

I "reviewed" and "approved" this pr, does this mean you can merge? If yes, please do whenever you like.

mofeing and others added 4 commits January 5, 2024 12:27
@mofeing
Copy link
Author

mofeing commented Jan 5, 2024

i've refactored some stuff and now is ready to go!
@jumerckx unfortunately i don't have write access so I can't merge

@jumerckx jumerckx merged commit 8e1c9c9 into jumerckx:vc/tblgen Jan 5, 2024
@mofeing mofeing deleted the build-script branch January 5, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants