Page MenuHomePhabricator

[MLIR] Make MLIR cmake variable names consistent
ClosedPublic

Authored by bondhugula on May 22 2021, 10:47 PM.

Details

Summary

Fix inconsistent MLIR CMake variable names. Consistently name them as
MLIR_ENABLE_<feature>.

Eg: MLIR_CUDA_RUNNER_ENABLED -> MLIR_ENABLE_CUDA_RUNNER

MLIR follows (or has mostly followed) the convention of naming
cmake enabling variables in the from MLIR_ENABLE_... etc. Using a
convention here is easy and also important for convenience. A counter
pattern was started with variables named MLIR_..._ENABLED. This led to a
sequence of related counter patterns: MLIR_CUDA_RUNNER_ENABLED,
MLIR_ROCM_RUNNER_ENABLED, etc.. From a naming standpoint, the imperative
form is more meaningful. Additional discussion at:
https://llvm.discourse.group/t/mlir-cmake-enable-variable-naming-convention/3520

Switch all inconsistent ones to the ENABLE form. Keep the couple of old
mappings needed until buildbot config is migrated.

Diff Detail

Event Timeline

bondhugula created this revision.May 22 2021, 10:47 PM
bondhugula requested review of this revision.May 22 2021, 10:47 PM
Herald added a project: Restricted Project. · View Herald Transcript

Simplify cmake var check.

mlir/CMakeLists.txt
115

I don't think this will actually override the cached setting. I think the more robust idiom is to move this above Ave use it to define a MLIR_ENABLE_BINDINGS_PYTHON_DEFAULT. And use that as the default value for the new cache variable.

There are some examples of this idiom in the root llvm cmake file.

bondhugula marked an inline comment as done.

Address review comments.

mlir/CMakeLists.txt
115

Thanks. A normal CMake variable actually hides any cache variables, but it isn't good practice to have a normal and cache variable with the same name. https://cmake.org/cmake/help/v3.0/command/set.html#set
I've now fixed it I believe in the way you are recommending.

stellaraccident accepted this revision.May 23 2021, 6:44 PM
stellaraccident added inline comments.
mlir/CMakeLists.txt
115

Thanks, and yes, I've washed a bunch of time with the shadowing in the past and try to avoid it.

This revision is now accepted and ready to land.May 23 2021, 6:44 PM