Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move internal changes #455

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Move internal changes #455

merged 1 commit into from
Dec 18, 2024

Conversation

christopherbate
Copy link
Collaborator

@christopherbate christopherbate commented Dec 17, 2024

NFC: Simplify some aspects of options management (OptionsContext)

  • Adds a convenience 'OptionsContext::Option' class that simplifies how
    options are declared.

  • Closes a loophole where tuples of structs containing options can cause
    crashes if they populate options in their constructor. Due to how the
    external storage mechanism works, we can no longer use direct std::tuple
    of aggregate objects which invoke addOption. Instead, one must use
    unique_ptr to wrap those types when used as elements of a std::tuple.

  • To help enforce this, we explicitly delete the move constructor of
    OptionsProvider.

[compiler|python] Update how cached pipelines/"Compiler Tasks" are registered

This change updates how registration functions for "compilation tasks"
invoked. We now expose a C API method that can be invoked within the
Pybind11 module initializer. This decouples compiler task registration from
pass or dialect registration.

This change also cleans up the C API function naming for pass/dialect
registration functions.

[python] Add more robust CMake logic for fixing missing CAPI dependency in core MLIR PyBind module

Adds CMake logic to ensure that the Core '_mlir' pybind extension has the
correct CAPI dependencies declared until the upstream CMake declarations can be
fixed.

NFC: Remove unnecessary PyCapsule <-> CAPI casters in compiler and runtime bindings

Removes unnecessary custom PyBind11 capsule -> C API object casters.
These cast functions are only required when it is desired to allow
PyBind11 to extract the C API object from the C++ python wrapper type
automatically.

[tensorrt|compiler] Drop "layer metadata callback" utility from TensorRT translation

This change removes the "layer metadata callback" feature from the
MLIR-to-TensorRT translation. It also removes the relevant APIs from the
MLIR-TensorRT compiler's C++ and Python APIs.

This capability was originally offered as a bridge for populating the
generated TensorRT ILayers with custom metadata. However, the mechanism
prevents caching of pass pipelines and therefore is too expensive to use.

In the future, any metadata passed to TensorRT should be derived from
the MLIR operations' location information.

NFC: update various uses of "Stablehlo" in class and function names to have consistent capitalization

NFC: Reorganize some directories

This change:

  • Moves the top-level 'tools' to 'compiler/tools'
  • Moves the top-level 'test' to 'compiler/test'
  • Moves the 'mlir-tensorrt-tblgen' tool under 'tensorrt/tools'
    since the 'tensorrt' project is supposed to be independent.
  • Similarly move TensorRT-specific python definitions under tensorrt/python.

[executor]: Add a missing guard for builds without CUDA enabled.

Wrapping the makeCudaStringError function with MLIR_EXECUTOR_ENABLE_CUDA fixes builds without CUDA enabled.

[executor] Use Lua locals for block arguments

Previously, the Executor MLIR-to-Lua translator used Lua globals for
block arguments outside of the entry block since the values that
represent block arguments need to be passed between blocks. On the
other hand, the scope of Lua local variables are restricted to their
block. It is almost never a good idea to use Lua global variables in
our translation strategy, however -- for coroutine functions, a
translation that uses globals is obviously incorrect since all Lua
coroutines in a single Lua environment share the same set of globals.

This change declares all block arguments up front as locals in the
"entry block" and just sets them to nil initially. Since we don't
declare a block scope for the entry block, all the following Lua block
scopes will have these locals in scope. This allows us to retain the
use of locals for all block arguments.

## NFC: Simplify some aspects of options management (OptionsContext)

- Adds a convenience 'OptionsContext::Option' class that simplifies how
  options are declared.

- Closes a loophole where tuples of structs containing options can cause
  crashes if they populate options in their constructor. Due to how the
  external storage mechanism works, we can no longer use direct `std::tuple`
  of aggregate objects which invoke `addOption`. Instead, one must use
  `unique_ptr` to wrap those types when used as elements of a `std::tuple`.

- To help enforce this, we explicitly delete the move constructor of
  `OptionsProvider`.

## [compiler|python] Update how cached pipelines/"Compiler Tasks" are registered

This change updates how registration functions for  "compilation tasks"
invoked. We now expose a C API method that can be invoked within the
Pybind11 module initializer. This decouples compiler task registration from
pass or dialect registration.

This change also cleans up the C API function naming for pass/dialect
registration functions.

## [python] Add more robust CMake logic for fixing missing CAPI dependency in core MLIR PyBind module

Adds CMake logic to ensure that the Core '_mlir' pybind extension has the
correct CAPI dependencies declared until the upstream CMake declarations can be
fixed.

## NFC: Remove unnecessary PyCapsule <-> CAPI casters in compiler and runtime bindings

Removes unnecessary custom PyBind11 capsule -> C API object casters.
These cast functions are only required when it is desired to allow
PyBind11 to extract the C API object from the C++ python wrapper type
automatically.

## [tensorrt|compiler] Drop "layer metadata callback" utility from TensorRT translation

This change removes the "layer metadata callback" feature from the
MLIR-to-TensorRT translation. It also removes the relevant APIs from the
MLIR-TensorRT compiler's C++ and Python APIs.

This capability was originally offered as a bridge for populating the
generated TensorRT ILayers with custom metadata. However, the mechanism
prevents caching of pass pipelines and therefore is too expensive to use.

In the future, any metadata passed to TensorRT should be derived from
the MLIR operations' location information.

## NFC: update various uses of "Stablehlo" in class and function names to have consistent capitalization

## NFC: Reorganize some directories

This change:

- Moves the top-level 'tools' to 'compiler/tools'
- Moves the top-level 'test' to 'compiler/test'
- Moves the 'mlir-tensorrt-tblgen' tool under 'tensorrt/tools'
  since the 'tensorrt' project is supposed to be independent.
- Similarly move TensorRT-specific python definitions under `tensorrt/python`.

## [executor]: Add a missing guard for builds without CUDA enabled.

Wrapping the makeCudaStringError function with MLIR_EXECUTOR_ENABLE_CUDA fixes builds without CUDA enabled.

## [executor] Use Lua locals for block arguments

Previously, the Executor MLIR-to-Lua translator used Lua globals for
block arguments outside of the entry block since the values that
represent block arguments need to be passed between blocks. On the
other hand, the scope of Lua local variables are restricted to their
block. It is almost never a good idea to use Lua global variables in
our translation strategy, however -- for coroutine functions, a
translation that uses globals is obviously incorrect since all Lua
coroutines in a single Lua environment share the same set of globals.

This change declares all block arguments up front as locals in the
"entry block" and just sets them to `nil` initially. Since we don't
declare a block scope for the entry block, all the following Lua block
scopes will have these locals in scope. This allows us to retain the
use of locals for all block arguments.

GitOrigin-RevId: e9dd03c47eab6145e889ea8ff56fd1c71181f72a
@christopherbate christopherbate merged commit de846c5 into main Dec 18, 2024
1 check passed
@christopherbate christopherbate deleted the move_internal_changes branch December 18, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants