Promote HAL driver and target backend CMake variables to options. by ScottTodd · Pull Request #7936 · iree-org/iree

ScottTodd · 2021-12-21T23:58:30Z

Following up on #7905 (comment), this uses regular options for these settings, which are easier to search for and set. The CMakeLists from Tint was used as a reference: https://dawn.googlesource.com/tint/+/refs/heads/main/CMakeLists.txt. The main focus is to use explicit names instead of dynamic name construction, but using option is a nice bonus.

New IREE_HAL_DRIVER_DEFAULTS and IREE_TARGET_BACKEND_DEFAULTS options can be used to set only a specific subset (set to OFF by default, then enable just the features you want).

Also fully removed the list helpers. They would be useful to add back in some form though...

GMNGeoffrey · 2021-12-22T00:04:17Z

-  ${CMAKE_CURRENT_LIST_DIR}/build_tools/cmake/
-  ${CMAKE_CURRENT_LIST_DIR}/bindings/python/build_tools/cmake/
-)
+option(IREE_HAL_DRIVER_CUDA "Enables the CUDA runtime HAL driver" ${IREE_HAL_DRIVER_CUDA_DEFAULT})


Whooo! This has always sketched me out also

As noted in Ben's comment, I think that having one easy variable to set things can still be nice maybe. But I think more important: it gave the ability to list exactly which drivers you wanted to build. If I only want Dylib, it's pretty annoying to have to list every single thing I want to disable, especially if that set changes over time. The single string variable gave that option, though it also was not super ergonomic to use. Another way to achieve that would be to have a single variable for setting the default for all "standard" drivers, which defaults to ON, then I can set it to OFF and only enable the one driver I want.

Something like

option(IREE_HAL_DRIVER_DEFAULT "Sets the default value for all standard drivers. Is overridden by platform-specific logic (e.g. disable cuda on android) and explicitly setting the variable for a specific driver" ON) if (APPLE) set(IREE_HAL_DRIVER_VULKAN_DEFAULT OFF) else() set(IREE_HAL_DRIVER_VULKAN_DEFAULT ${IREE_HAL_DRIVER_DEFAULT}) endif() option(IREE_HAL_DRIVER_DYLIB "Enables the Dylib runtime HAL driver" ${IREE_HAL_DRIVER_DEFAULT})

Nice, overriding the default is pretty clean. Thanks for the suggestion!

GMNGeoffrey · 2021-12-22T00:14:02Z

+# Disable WebGPU by default - it has complex deps and is under development.
+option(IREE_TARGET_BACKEND_WEBGPU "Enables the WebGPU compiler target backend" OFF)
+
+message(STATUS "IREE build runtime HAL driver 'CUDA': ${IREE_HAL_DRIVER_CUDA}")


This is a lot of repetition. Could we have a little macro/function that does both the option creation and the status message? Would still definitely want the variable names as literals. Something like:

function(OPTION NAME DEFAULT) option(${OPTION} "Enables the '${NAME}' compiler target backend") message(STATUS "IREE build compiler target backend '${NAME}': ${OPTION}") endfunction()

I actually like the repetition here, as a user of the project. No logic to follow, just WYSIWYG :P

Elaborating on this a bit:

When integrating a project that uses CMake, the root CMakeLists.txt is the first (and sometimes only) place to look. It is very helpful if that file is organized into sections and has easy to follow logic. I don't think wrapping this in a function offers much benefit for maintainers (find/replace and multicursor editing within 40 contiguous lines of code is easy), and having each option spelled out with no loops, branches, or string manipulation makes it easy for users to scan the file and see which options they may want to enable/disable for their use.

GMNGeoffrey · 2021-12-22T00:18:09Z

            "-B", hostBuildDir ,
-            "-DIREE_TARGET_BACKENDS_TO_BUILD=vmvx",
-            "-DIREE_HAL_DRIVERS_TO_BUILD=vmvx",
+            "-DIREE_HAL_DRIVER_VMVX=ON"


Yeah so this case is exactly what I was talking about above :-) Not terrible for CI, but imagine being a human

🤖 haha yeah, imagine being a human... 👀

…tes.

benvanik · 2021-12-22T22:15:11Z

-
-if(IREE_HAL_DRIVERS_TO_BUILD STREQUAL "default")
-  set(IREE_HAL_DRIVERS_TO_BUILD ${IREE_ALL_HAL_DRIVERS})
+option(IREE_HAL_DRIVER_DEFAULT "Sets the default value for all runtime HAL drivers" ON)


worth making this plural (DEFAULTS) or something - "IREE_HAL_DRIVER_DEFAULT" makes me think it's picking a default driver, not that it's using the set of default drivers.

SG, changed to plural.

benvanik · 2021-12-22T22:16:49Z

+# Disable WebGPU by default - it has complex deps and is under development.
+option(IREE_TARGET_BACKEND_WEBGPU "Enables the 'webgpu' compiler target backend" OFF)
+
+message(STATUS "IREE build runtime HAL driver 'cuda': ${IREE_HAL_DRIVER_CUDA}")


do we need this? feels a bit spammy - especially when we are used as part of a larger project - VERBOSE seems like a better category if we do have it

Changed to verbose. I like having it, since a few settings do have some logic (CUDA/Vulkan), and having the full list is useful in bug reports.

BTW, VERBOSE is new in CMake 3.15: https://cmake.org/cmake/help/latest/command/message.html. We require 3.16.3+: cmake_minimum_required(VERSION 3.16.3...3.21)

log level can be controlled with --log-level=<ERROR|WARNING|NOTICE|STATUS|VERBOSE|DEBUG|TRACE> to the cmake command or with the CMAKE_MESSAGE_LOG_LEVEL option

benvanik · 2021-12-22T22:17:22Z

 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

-if(NOT ${IREE_TARGET_BACKEND_DYLIB-LLVM-AOT}
+if(NOT ${IREE_TARGET_BACKEND_DYLIB_LLVM_AOT}


yess thank you!

Remove two obsolete iree flags IREE_TARGET_BACKENDS_TO_BUILD and IREE_HAL_DRIVERS_TO_BUILD. They were removed from IREE codebase at iree-org/iree#7936. Change-Id: If505172caca03a0c5ee4c922ad4cc8548760a013 GitOrigin-RevId: 4c10699fcb81d824e4945d21f80ceada09aacff0

Remove two obsolete iree flags IREE_TARGET_BACKENDS_TO_BUILD and IREE_HAL_DRIVERS_TO_BUILD. They were removed from IREE codebase at iree-org/iree#7936. Change-Id: If505172caca03a0c5ee4c922ad4cc8548760a013

Promote HAL driver and target backend CMake variables to options.

a7d44a4

Also fully removed the list helpers. They would be useful to add back in some form though...

ScottTodd added infrastructure/cmake cleanup 🧹 labels Dec 21, 2021

ScottTodd requested review from GMNGeoffrey and benvanik December 21, 2021 23:58

ScottTodd commented Dec 22, 2021

View reviewed changes

Comment thread CMakeLists.txt Outdated

GMNGeoffrey reviewed Dec 22, 2021

View reviewed changes

ScottTodd added 5 commits December 22, 2021 09:28

Add default control options to make enabling a specific subset easier.

85a5f34

Lowercase CMake driver/target labels to match names in code.

013a21f

Replace dashes with underscores in CMake option names.

2d3c0c6

Fix up references to IREE_HAL_DRIVERS_TO_BUILD.

f12cb24

Replace - with _ when evaluating target/driver exclusions in test sui…

639fe2f

…tes.

GMNGeoffrey approved these changes Dec 22, 2021

View reviewed changes

Comment thread docs/developers/get_started/cmake_options_and_variables.md Outdated

Comment thread iree/samples/static_library/README.md

Document defaults, fix riscv toolchain, flip defaults in static library.

5fe59f3

benvanik approved these changes Dec 22, 2021

View reviewed changes

ScottTodd added 2 commits December 22, 2021 14:23

Plural "_DEFAULTS".

f6bf384

Change messages from STATUS -> VERBOSE.

912385e

ScottTodd enabled auto-merge (squash) December 22, 2021 22:29

ScottTodd merged commit d1620f0 into iree-org:main Dec 22, 2021

ScottTodd deleted the cmake-target-options branch December 22, 2021 22:55

GMNGeoffrey mentioned this pull request Dec 23, 2021

Merge main -> google #7947

Merged

ScottTodd mentioned this pull request Mar 1, 2022

Rework cmake test deps, target selection and presets. #8423

Merged

GMNGeoffrey added the infrastructure Relating to build systems, CI, or testing label Jul 19, 2022

Uh oh!

Conversation

ScottTodd commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ScottTodd commented Dec 21, 2021 •

edited

Loading