-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
Add MLIR package to llvm 17 #280572
Add MLIR package to llvm 17 #280572
Conversation
475e114
to
917015c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits
nativeBuildInputs = [ cmake ninja ]; | ||
buildInputs = [ libllvm libxml2 ]; | ||
|
||
ninjaFlags = [ "-v " ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
# Disables building of shared libs, -fPIC is still injected by cc-wrapper | ||
"-DLLVM_ENABLE_PIC=OFF" | ||
"-DLLVM_BUILD_STATIC=ON" | ||
"-DLLVM_LINK_LLVM_DYLIB=off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those cmake options case-sensitive?
inherit version doCheck; | ||
|
||
# Blank llvm dir just so relative path works | ||
src = runCommand "${pname}-src-${version}" {} '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src = runCommand "${pname}-src-${version}" {} '' | |
src = runCommand "mlir-src-${version}" {} '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the same pattern from other packages.
mkdir -p "$out/llvm" | ||
''; | ||
|
||
sourceRoot = "${src.name}/${pname}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sourceRoot = "${src.name}/${pname}"; | |
sourceRoot = "${src.name}/mlir"; |
Can it be backported to 23.11 too? Thanks! |
Gentle ping |
I rather to keep it here, since it needs a lot of testing and moving to a stable branch right now might not be a good idea |
|
||
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY | ||
- "${CMAKE_CURRENT_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}") | ||
- set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need this? My recollection is the CURRENT
/ RUNTIME
stuff is fine to keep as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know to be honest, I followed the same pattern from other llvm components, I'll try again without this and if it works I'll remove the change. Otherwise, I'll keep it and mark it as resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sounds good.
Yes some of the other patches are a bit stale as opposed to what I actually upstreamed, and some things maybe were intentionally not upstreamed because it turned out they weren't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this patch and built mlir. While it builds, but programs that target mlir fail to find it. To be exact, cmake fails to find mlir-tblgen
but with this patch the same program compiles with no issue. So I think we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a test for this? OK if it is in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure
Co-authored-by: John Ericson <[email protected]>
Co-authored-by: John Ericson <[email protected]>
OK great this looks good! Can you add it to |
I'll add another PR for the git version and another one for tests. |
Great, thanks! |
Successfully created backport PR for |
"-DLLVM_INSTALL_TOOLCHAIN_ONLY=OFF" | ||
"-DMLIR_TOOLS_INSTALL_DIR=${placeholder "out"}/bin/" | ||
"-DLLVM_ENABLE_IDE=OFF" | ||
"-DLLD_INSTALL_PACKAGE_DIR=${placeholder "out"}/lib/cmake/mlir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this should be MLIR_INSTALL_PACKAGE_DIR
not LLD_INSTALL_PACKAGE_DIR
?
(Thanks for working on this!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good point
|
It is available as |
Oh sorry! |
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-23.11
git worktree add -d .worktree/backport-280572-to-release-23.11 origin/release-23.11
cd .worktree/backport-280572-to-release-23.11
git switch --create backport-280572-to-release-23.11
git cherry-pick -x 262ed9bd904310a33653904a56546a15f88e971a 777f8c6a12e3d2c7a3732618accfc29395432be0 da2ccc5719c79b28f4bacd20c20ce897e87f605b |
This PR adds the mlir package to llvm 17.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.