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

[RTG][python] fix populateDialectRTGTestSubmodule namespacing #8078

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

makslevental
Copy link
Contributor

If CIRCT_INCLUDE_TESTS=OFF but this source file is compiled (only to eventually be DCEd) circt::python::populateDialectRTGTestSubmodule is malformed. With explicit namespace circt::python it works both ways.

@makslevental makslevental requested a review from maerhart January 13, 2025 16:06
@maerhart
Copy link
Member

Given CIRCT_INCLUDE_TESTS=OFF, is there actually a situation in which this cpp file is compiled? Because there shouldn't. The CMakeLists only includes this file if tests are enabled.

Wouldn't compilation with CIRCT_INCLUDE_TESTS=ON still fail with this change because the RTGTest CAPI used in there also wasn't compiled?

We could enclose the function definition in an #ifdef CIRCT_INCLUDE_TESTS to make it robust against that. My thought was that not having it leads to a linker error pointing at a misconfigured build system (CMake) file.

@makslevental
Copy link
Contributor Author

Given CIRCT_INCLUDE_TESTS=OFF, is there actually a situation in which this cpp file is compiled? Because there shouldn't. The CMakeLists only includes this file if tests are enabled.

admittedly I'm doing something unconventional - I'm building y'alls Python bindings but downstream of you. In such a setup, lib/Bindings/Python/RTGTestModule.cpp gets added to the CIRCTBindingsPythonExtension target (if CIRCT_INCLUDE_TESTS was on during build). Then when that same target is used in the downstream project it's compiled (even if CIRCT_INCLUDE_TESTS is off).

Wouldn't compilation with CIRCT_INCLUDE_TESTS=ON still fail with this change because the RTGTest CAPI used in there also wasn't compiled?

Nah because something like rtgtestCPUTypeGet is only exposed through your circt-c/Dialect/RTGTest.h header and then since populateDialectRTGTestSubmodule is never called (because that is already behind #ifdef CIRCT_INCLUDE_TESTS), the linker DCEs everything.

Anyway this is actually moot (I don't need to build all of CIRCT's dialects so I don't need this module at all actually). Up to you if you want to me to get this across the finish line or just close.

Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

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

This change looks innocuous enough to me, though the discussion about whether or not it's compiled should be resolved into an issue or not.

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.

3 participants