MLIR cmake enable variable naming convention

For CMake variables, LLVM and MLIR follow (or have mostly followed) the convention of naming enabling variables LLVM_ENABLE_..., MLIR_ENABLE_... etc… Using a convention here is easy and also important for convenience. However, a counter pattern was started with some variables being named MLIR_<something>_ENABLED. This led to a sequence of other counter patterns for closely related things - for eg., MLIR_CUDA_RUNNER_ENABLED, MLIR_ROCM_RUNNER_ENABLED, MLIR_SPIRV_CPU_RUNNER_ENABLED, and more like MLIR_PYTHON_BINDINGS_ENABLED. This has become pretty annoying because a developer has to pretty much try/guess or lookup each flag to see if it follows the _ENABLED or ENABLE conventions – esp. when quickly switching.

From a naming standpoint itself, the imperative form is more meaningful — we are enabling the said feature during build generation; it’s not already enabled. I think it’s important to fix the convention early instead of creating two parallel ones and having a larger impact downstream later.

There are also inconsistencies like this one right next to each other:

set(MLIR_ENABLE_CUDA_CONVERSIONS 1)
...
add_definitions(-DMLIR_ENABLE_CUDA_CONVERSIONS=${MLIR_CUDA_CONVERSIONS_ENABLED})
...
set(MLIR_ROCM_CONVERSIONS_ENABLED 1)
...
set(MLIR_BINDINGS_PYTHON_ENABLED 0 CACHE BOOL
       "Enables building of Python bindings.")
set(MLIR_PYTHON_BINDINGS_VERSION_LOCKED 1 CACHE BOOL
       "Links to specific python libraries, resolving all symbols.")

For those like the bindings ones, they can be resolved to align with the directory structure - i.e., going with MLIR_ENABLE_BINDINGS_PYTHON....

I propose to make everything uniform to use MLIR_ENABLE_... if everyone agrees.

1 Like

+1 - I’ve been annoyed by this but not enough to normalize it. Thank you.

+1 for me as well. LLVM is annoyingly inconsistent with naming, but it is still time to fix it in MLIR I think!

To roll this out, you likely want to support both the old and the new name at first, and update the buildbots configs in GitHub - llvm/llvm-zorg before removing the old names

+1 I hadn’t seen this creep in, but definitely worth fixing.

Thanks! Here are the revisions:

  1. ⚙ D102976 [MLIR] Make MLIR cmake variable names consistent
  2. ⚙ D102977 [MLIR] Update MLIR build config to reflect cmake variable renames

A third one will remove the couple of old ones preserved for buildbot.

⚙ D102997 [MLIR] Drop old cmake var namees now takes care of dropping the couple of old ones that had been preserved for buildbot.