[mlir][CMake] enable disabling MLIR_ENABLE_EXECUTION_ENGINE

[makslevental]

Currently if you pass MLIR_ENABLE_EXECUTION_ENGINE=OFF it's overwritten.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

Currently if you pass MLIR_ENABLE_EXECUTION_ENGINE=OFF it's overwritten.

---

Full diff: https://github.com/llvm/llvm-project/pull/171060.diff

1 Files Affected:

• (modified) mlir/CMakeLists.txt (+5-2)

diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index 570fb6f89dd1c..9e1e9314511e3 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -124,10 +124,13 @@ set_target_properties(mlir-doc PROPERTIES FOLDER "MLIR/Docs")
 
 # Only enable execution engine if the native target is available.
 if(${LLVM_NATIVE_ARCH} IN_LIST LLVM_TARGETS_TO_BUILD)
-  set(MLIR_ENABLE_EXECUTION_ENGINE 1)
+  set(MLIR_ENABLE_EXECUTION_ENGINE_default 1)
 else()
-  set(MLIR_ENABLE_EXECUTION_ENGINE 0)
+  set(MLIR_ENABLE_EXECUTION_ENGINE_default 0)
 endif()
+option(MLIR_ENABLE_EXECUTION_ENGINE
+       "Enable building the MLIR Execution Engine."
+       ${MLIR_ENABLE_EXECUTION_ENGINE_default})
 
 # Build the ROCm conversions and run according tests if the AMDGPU backend
 # is available.

[joker-eph]

Can you clarify why we want this as an option?

[makslevental]

Can you clarify why we want this as an option?

Because you can't always use (or even build) the ExecutionEngine? E.g., on wasm32-emscripten nothing will work (not our memref ABI, which uses int64_t everywhere, nor OrcJIT). So at best it's added compile cost (for such a target/platform) for no benefit, and at worst it prevents building - I found this due to hitting

em++: error: undefined exported symbol: "_LLVMAddSymbol" [-Wundefined] [-Werror]

when linking mlir_runner_utils and mlir_c_runner_utils (wasm-ld for whatever reason decides to prune that symbol 🤷).

[joker-eph]

Can we instead improve the logic to detect that this is an unsupported target, and disable the execution engine (with a warning maybe), that would be more in line with the existing logic.

Someone hitting that linking error you mentioned wouldn't have a clue about what to do to make their build work otherwise.

misclick

[makslevental]

Can we instead improve the logic to detect that this is an unsupported target, and disable the execution engine (with a warning maybe), that would be more in line with the existing logic.

It's a top-level CMake variable - I don't understand what the issue is with giving people the choice to disable it? I don't see the difference between that variable and these which are fully user settable

llvm-project/mlir/CMakeLists.txt

Lines 148 to 156 in ebdb903

	set(MLIR_ENABLE_CUDA_RUNNER 0 CACHE BOOL "Enable building the MLIR CUDA runner")
	set(MLIR_ENABLE_ROCM_RUNNER 0 CACHE BOOL "Enable building the MLIR ROCm runner")
	set(MLIR_ENABLE_SYCL_RUNNER 0 CACHE BOOL "Enable building the MLIR SYCL runner")
	set(MLIR_ENABLE_LEVELZERO_RUNNER 0 CACHE BOOL "Enable building the MLIR LevelZero runner")
	set(MLIR_ENABLE_SPIRV_CPU_RUNNER 0 CACHE BOOL "Enable building the MLIR SPIR-V cpu runner")
	set(MLIR_ENABLE_VULKAN_RUNNER 0 CACHE BOOL "Enable building the MLIR Vulkan runner")
	set(MLIR_ENABLE_NVPTXCOMPILER 0 CACHE BOOL
	"Statically link the nvptxlibrary instead of calling ptxas as a subprocess \
	for compiling PTX to cubin")

Someone hitting that linking error you mentioned wouldn't have a clue about what to do to make their build work otherwise.

That linking error is a wasm-ld issue - it turned out to be a red-herring. But my point about unnecessary compile cost on wasm32 (or anywhere else people don't want/use ExecutionEngine) still stands.

[rengolin]

Not being able to control whether we build the execution engine or not is bad design, especially in the face where some targets out there that we don't test don't work properly. We have many other cases of components that we allow to build/not-build, so this won't be the first.

Furthermore, I have to say, the idea that we can always enable the execution engine for whatever target is "native" was a pretty naive choice to begin with. It's more stable to enable targets that we know are supported than disable the ones we have found (on arbitrary environments) to be unsupported.

Both new components policy and the community support policy encode code support as opt-in, not opt-out.

That's because when someone opts in, they are given the responsibility to keep it working, and will be responsible to fix when things break. An opt-out strategy will not have the same guarantees.

We cannot ask maintainers of the execution engine to make sure it works for every possible native target out there, nor to maintain a list of potentially broken targets that they don't have access to and still not provide a way to override.

To avoid breaking changes with the previous behaviour, I think this PR solves it in a reasonable way. It doesn't change the current behaviour but allows builds to override the default behaviour.

Longer term we may want a better default control at the CMake level, but I don't think this should hold this PR from merging, and being worked later. At the very least, this PR would allow people to build and test different environments and options, so we can design a better solution in the end.

But it can all start with this override, and then a longer term design in follow up PRs.

[joker-eph]

One high-level note: please don't merge PR before ensuring that reviewers are OK with it when there are on-going discussions, like here. Having another reviewer approving a PR does not preclude from finishing discussion with other reviewers.

It's a top-level CMake variable - I don't understand what the issue is with giving people the choice to disable it?

There is no "issue" per-se: it's all a question of motivation and tradeoff.
MLIR is very modular in that you only need to link what you need and we try to not "pollute" your final binary with useless code, but we don't go the extra mile with a gazillion CMake option to avoid building some components by default. It increases the support surface and makes incredibly difficult to support the combination of these. So in the end it is a "ROI" kind of concern.

The existing logic is present because we detect an "incorrect" configuration where the JIT cannot work without the host backend available: we're trying to avoid people having weird errors.
Hence the angle I took in my answer: the motivation you had express with your change was because of a configuration that leads to errors that could be "surprising" to the user, and hard for them to figure out what to do to unblock themselves.
In general my take in MLIR has been to try to catch these scenario earlier and diagnose the problem close to the source, where we can indicate which flag to tweak for example.

For a different motivation, I would answer differently, and right now the PR description does not include any unfortunately.

I don't see the difference between that variable and these which are fully user settable

There is a fundamental difference in one that is "hermetic" to the LLVM build system (the same invocation gives the same results) and one where depending on whether cmake detects the right cuda or python you end up with a different build configuration.