[RTG][python] fix populateDialectRTGTestSubmodule namespacing

[makslevental]

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.

[maerhart]

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]

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.

[teqdruid]

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.

[youngar]

I am running in to this issue as well, but I don't think this is the right fix. LLVM has a coding standard against this, as it should really be a compilation error.

Here is what I am seeing: declare_mlir_python_extension creates an interface library. This means that each time you link against this library, it will rebuild the sources. This source is supposed to be built with -DCIRCT_INCLUDE_TESTS, but for some reason, it is missing the flag when built by a downstream project.

In the top level cmake file for circt, we add this flag to all libraries:

circt/CMakeLists.txt

Lines 608 to 610 in c67ae28

	if (CIRCT_INCLUDE_TESTS)
	add_definitions(-DCIRCT_INCLUDE_TESTS)
	endif()

I personally would have expected the interface library to add this definition, so that when the sources get built it includes this flag. I was able to get this working by adding the definition to the interface library manually in lib/Bindings/Python/CMakeLists.txt

if (CIRCT_INCLUDE_TESTS)
  target_compile_definitions(CIRCTBindingsPythonExtension INTERFACE CIRCT_INCLUDE_TESTS)
endif()

I tried two versions of cmake, 3.23 and 3.27, and saw the issue. @maerhart does not see this issue using version 3.31.3.