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

Add MLIR package to llvm 17 #280572

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Add MLIR package to llvm 17 #280572

merged 3 commits into from
Feb 22, 2024

Conversation

lxsameer
Copy link
Contributor

This PR adds the mlir package to llvm 17.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@lxsameer lxsameer force-pushed the mlir-17 branch 2 times, most recently from 475e114 to 917015c Compare January 12, 2024 20:25
Copy link
Member

@AndersonTorres AndersonTorres left a 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 " ];
Copy link
Member

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"
Copy link
Member

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}" {} ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
src = runCommand "${pname}-src-${version}" {} ''
src = runCommand "mlir-src-${version}" {} ''

Copy link
Contributor Author

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}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#278611

Suggested change
sourceRoot = "${src.name}/${pname}";
sourceRoot = "${src.name}/mlir";

@wegank wegank marked this pull request as draft January 21, 2024 23:20
@wegank wegank changed the base branch from staging to master January 21, 2024 23:21
@wegank wegank marked this pull request as ready for review January 21, 2024 23:21
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jan 22, 2024
@timothyklim
Copy link
Contributor

Can it be backported to 23.11 too? Thanks!

@timothyklim
Copy link
Contributor

Gentle ping

@lxsameer
Copy link
Contributor Author

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")
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure

@ofborg ofborg bot requested a review from Ericson2314 February 20, 2024 17:23
@ofborg ofborg bot requested a review from Ericson2314 February 21, 2024 14:49
@Ericson2314 Ericson2314 merged commit 350aae6 into NixOS:master Feb 22, 2024
22 of 23 checks passed
@Ericson2314
Copy link
Member

OK great this looks good! Can you add it to llvmPackages_git too?

@lxsameer
Copy link
Contributor Author

I'll add another PR for the git version and another one for tests.

@Ericson2314
Copy link
Member

Great, thanks!

Copy link
Contributor

Successfully created backport PR for release-23.11:

"-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"

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!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 1, 2024

Can we get this for 18, recently added, and git too, so we don't get out of sync?

@wegank
Copy link
Member

wegank commented Apr 1, 2024

It is available as llvmPackages_18.mlir and llvmPackages_git.mlir.

@Ericson2314
Copy link
Member

Oh sorry!

@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Copy link
Contributor

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants