[CMake] Implement add_circt_tool()

[nickelpro]

Right now we use add_llvm_tool() to add tools, which is equivalent to llvm_add_tool(LLVM ${ARGV}). This puts packagers of CIRCT in the somewhat awkward situation of needing to use LLVM_TOOLS_INSTALL_DIR to specify where CIRCT tools should be placed in the install tree.

CIRCT is not LLVM.

This patch adds a specialization of llvm_add_tool(). This is the same approach used by MLIR and LLVM itself.

[dtzSiFive]

Good step forward. Unfortunately I don't think MLIR does this the way we want (their targets for tools aren't exported under MLIR, they get added as LLVM exports, see https://llvm.org/PR52334, tools need you to manually set LLVM_BUILD_TOOLS to be included/installed by default, vs other projects having their own PROJECT_BUILD_TOOLS); maybe related mlir-tblgen doesn't work when building downstream project against standalone-built MLIR, but there's other differences there)-- we should probably have our own implementation of llvm_add_tool that does the right thing for our project (which is what lld/clang/flang/bolt do).

Look at CIRCTTargets.cmake to see what I mean-- no executables .

Anyway, this is to enable building CIRCT as part of a unified build and directing its tools into a different directory than the others (LLVM bits), is that right?

[nickelpro]

Anyway, this is to enable building CIRCT as part of a unified build and directing its tools into a different directory than the others (LLVM bits), is that right?

Yes, but also for packaging hygiene. Even with a standalone build it's bizarre and unintuitive to pass -DLLVM_TOOLS_INSTALL_DIR to a CIRCT CML when no other LLVM project works that way. This patch was motivated by going through the process of packaging CIRCT and being caught off-guard by this.

we should probably have our own implementation of llvm_add_tool that does the right thing for our project (which is what lld/clang/flang/bolt do).

Happy to work on this, but don't think it should blocker for what I suspect is a tiny bug fix with very low exposure. I don't think many people are packaging CIRCT yet, and fixing this before that starts happening ensures that we won't break people's packaging scripts in the future.

Eh, quick look at lld suggests this should be quick. Will try this approach

[dtzSiFive]

Yes, but also for packaging hygiene. Even with a standalone build it's bizarre and unintuitive to pass -DLLVM_TOOLS_INSTALL_DIR to a CIRCT CML when no other LLVM project works that way. This patch was motivated by going through the process of packaging CIRCT and being caught off-guard by this.

That's super weird! This is needed for a standalone build?? That's no good, absolutely. Thanks for your interest and working on improving this!

I wonder why you're seeing that, I've been packaging CIRCT (standalone) for some time now and never had to specify this.
Anyway happy this fixes that for you, and seems like a solid change regardless.

If you'd like to investigate/work on some of the other bits that'd be swell, but absolutely that's not a blocker here 👍.

[nickelpro]

That's super weird! This is needed for a standalone build?? That's no good, absolutely. Thanks for your interest and working on improving this!

This is inherent to how llvm_add_tool() works, it looks up the tools dir based on the passed project name

Hilariously, LLD and friends screw this up. They set a ${PROJ_TOOLS_INSTALL_DIR} but do not use it anywhere. In their add_proj_tool() functions they just use ${CMAKE_INSTALL_BINDIR} ignoring the variable they set up.

MLIR doesn't screw this up because they don't roll their own add_mlir_tool(), and so benefit from the upstream correct LLVM version.

[nickelpro]

@dtzSiFive I believe this does what we want now. It generates export targets for the CIRCT tools under CIRCTTargets.cmake, it uses the CIRCT_INSTALL_TOOLS_DIR, and completely excludes both targets and tools if CIRCT_INCLUDE_TOOLS is set to OFF which mirrors LLVM behavior. This is nearly identical to the other tools (lld/flang/bolt), except for above-mentioned bug with how they handle the INSTALL_TOOLS_DIR.

The second commit is cleanliness. We don't use these variables (except something to do with PyCDE?) and they're the last references to LLVM vars in the top level CML. I'll drop it if desired.

[nickelpro]

Test are failing because check-circt can no longer find binaries under ${CMAKE_CURRENT_BINARY_DIR}/bin. They're now located under tools/tool_name, where they appear in the source tree.

But I don't actually know how CIRCT was getting cmake to relocate the binaries in the first place so I need to investigate. I'll say now the way that LLVM projects operate out of build trees is very alien to me.

[nickelpro]

This passes check-circt and the rest now.

llvm_add_executable() uses the LLVM_RUNTIME_OUTPUT_INTDIR and LLVM_LIBRARY_OUTPUT_INTDIR parent scope variables to re-path targets in the build tree which is, uh, certainly something.

Don't see a lot of value in writing our own add_executable() here, so restoring those variable to fix everything.

[teqdruid]

FYI: I'm about to remove esi-tester. If this PR gets merged promptly, I can wait.

[teqdruid]

CMake Error at /home/jodemme/FPGAToolsAndCompilers/CIRCT/circt/frontends/PyCDE/src/CMakeLists.txt:118 (install):
  install TARGETS given target "circt-std-sim-drivers" which does not exist.

When PyCDE is enabled. I spent a lot of time a few months ago figuring out the cmake stuff for PyCDE install, so please don't break it.

[nickelpro]

I didn't test the unified build 😔

Ok, more investigation to get this right. Crash coursing in the LLVM build environment right now, apologies for the false assurances this was ready

Don't delay anything on my account, rebasing this is easy

[nickelpro]

Stupid bug, CIRCT_INCLUDE_TOOLS and CIRCT_BUILD_TOOLS were only being defined for standalone builds, so defaulted to false-y for unified builds.

Fixed, unified builds work now. PyCDE builds clean.

So uh, run it again 🤞

[teqdruid]

Fixed, unified builds work now. PyCDE builds clean.

Yep. It also installs properly. Thanks!

[dtzSiFive]

LGTM, thank you!

[teqdruid]

This didn't break me and my policy when it comes to cmake changes is if it seems reasonable and doesn't break ppl it's ok since I don't know much about cmake. @dtzSiFive? it seems we collided.

[mikeurbach]

I have the same policy as John :) If this makes sense to Will, it makes sense to me.

[dtzSiFive]

Just pointed my nix packaging of CIRCT at this and works great for me! (as expected, but just went to confirm)

[dtzSiFive]

@nickelpro, would you like to land this (become a contributor), or have one of us do it for you?
You can request commit (push) access if you'd like following these instructions: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access .

Or we can merge this for you. Thank you for your contribution!

[nickelpro]

Shot Chris an email, but no reason to hold this up. Anyone can feel free to merge.

Might as well take this chance for brief introduction: My name's Vito, I run the NYU ProcDesign team. I personally mostly focus on packaging, toolchain, and frontends. Very new to MIRL and all involved. Going to be working with CIRCT for most of this year.

Thanks for fast feedback/approval 🎉