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

Make setup_sanitizer include optional in CMakeLists.txt #1972

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

sjain-stanford
Copy link
Contributor

Fixes #1962 (comment)

Context:
This breaks torch-mlir's CMake build when bumping stablehlo.

The CMake build fails with setup_sanitizers. Probably related to #1959 (cc: @fzakaria , @mlevesquedion ). I see the default for STABLEHLO_ENABLE_SANITIZER being OFF, but this code probably needs to be enclosed in an if-endif:

include(SetupSanitizers)
setup_sanitizers()

Error:

  -- Building StableHLO as an external LLVM project
  -- Building with -fPIC
  CMake Error at /_work/torch-mlir/torch-mlir/externals/stablehlo/CMakeLists.txt:147 (include):
    include could not find requested file:
  
      SetupSanitizers
  
  
  CMake Error at /_work/torch-mlir/torch-mlir/externals/stablehlo/CMakeLists.txt:148 (setup_sanitizers):
    Unknown CMake command "setup_sanitizers".
  
  
  -- Configuring incomplete, errors occurred!

@fzakaria
Copy link
Contributor

I think the change is to fix:

list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake")

to not be the PROJECT_SOURCE_DIR but a directory relative to the current file which would work in embedded use case.

@fzakaria
Copy link
Contributor

I will test a change using CMAKE_CURRENT_LIST_DIR so that it propagates.

@sjain-stanford
Copy link
Contributor Author

@fzakaria using CMAKE_CURRENT_LIST_DIR works too. Just pushed the change. Do you want me to still keep the if-endif (I've removed for now)?

@fzakaria
Copy link
Contributor

LGTM thanks!

Copy link
Contributor

@fzakaria fzakaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thank you for iterating a little on it.
You also confirmed it worked for the embedded case?

I'm new to the team and wasn't aware who is using that embedded case as well.

@sjain-stanford
Copy link
Contributor Author

sjain-stanford commented Jan 31, 2024

You also confirmed it worked for the embedded case?

I've tested it from torch-mlir where stablehlo is an external git submodule. If that's what you're referring here as the embedded case then yes!

@GleasonK GleasonK merged commit fd52182 into openxla:main Jan 31, 2024
9 checks passed
@sjain-stanford sjain-stanford deleted the sambhav/sanitizer_patch branch January 31, 2024 20:27
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.

TorchToStablehlo lit test failure with scatter.mlir
3 participants