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

BUG: build_native_tool does not pass on CMAKE_CXX_STANDARD #60574

Closed
h-vetinari opened this issue Feb 7, 2023 · 12 comments · Fixed by llvm/llvm-project-release-prs#284
Closed

Comments

@h-vetinari
Copy link
Contributor

Trying to build mlir 16.0.0-rc1 in conda-forge, I'm running into the issue that MLIR now requires C++17 to build, but specifying CMAKE_CXX_STANDARD to the build does not get respected in cross-compilation.

From what I can tell, this happens when invoking build_native_tool, which then fails on any compiler that doesn't yet default to C++17.

[49/1416] Building native mlir-tblgen...
[1/47] Building CXX object tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.o
FAILED: tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.o 
$BUILD_PREFIX/bin/x86_64-apple-darwin13.4.0-clang++ -DMLIR_CUDA_CONVERSIONS_ENABLED=1 -DMLIR_ROCM_CONVERSIONS_ENABLED=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I$SRC_DIR/build/NATIVE/tools/mlir-tblgen -I$SRC_DIR/mlir/tools/mlir-tblgen -I$SRC_DIR/mlir/include -I$SRC_DIR/build/NATIVE/include -isystem $BUILD_PREFIX/include -O2 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -Werror=mismatched-tags -O3 -DNDEBUG -isysroot /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -mmacosx-version-min=11.0  -fno-exceptions -MD -MT tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.o -MF tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.o.d -o tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.o -c $SRC_DIR/mlir/tools/mlir-tblgen/DirectiveCommonGen.cpp
In file included from $SRC_DIR/mlir/tools/mlir-tblgen/DirectiveCommonGen.cpp:14:
In file included from $SRC_DIR/mlir/include/mlir/TableGen/GenInfo.h:12:
In file included from $SRC_DIR/mlir/include/mlir/Support/LLVM.h:24:
$BUILD_PREFIX/include/llvm/Support/Casting.h:266:32: error: no member named 'optional' in namespace 'std'
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 7, 2023

@llvm/issue-subscribers-mlir

@joker-eph
Copy link
Collaborator

I don't quite get how this happens, the CMakeLists.txt will by default do set(CMAKE_CXX_STANDARD 17 ), why isn't this enough to get the right -std=c++17 used here?

@h-vetinari
Copy link
Contributor Author

why isn't this enough to get the right -std=c++17 used here?

TBH, I don't know myself, but I have the suspicion that it was masked by the fact that on new enough clang (and GCC) the default is already C++17. For various unhappy reasons we're still on clang 14 on osx.

@h-vetinari
Copy link
Contributor Author

I don't quite get how this happens, the CMakeLists.txt will by default do set(CMAKE_CXX_STANDARD 17 )

Ah, from what I can tell, this isn't actually happening in mlir/ itself (except in an example), which is the only folder that we're building (llvm is built separately before).

@joker-eph
Copy link
Collaborator

You're invoking CMake pointing directly at MLIR?
I forgot we can do that, but now that you say I kind of remember that you can build against an already built LLVM right?
We should just add the directive in the MLIR CMake then, can you try and let me know if it is enough?

@h-vetinari
Copy link
Contributor Author

You're invoking CMake pointing directly at MLIR?

Yes.

We should just add the directive in the MLIR CMake then, can you try and let me know if it is enough?

Can confirm it works with the following patch:

diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index f00c819b25..4594b8830e 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -22,6 +22,7 @@ endif()

 # Must go below project(..)
 include(GNUInstallDirs)
+set(CMAKE_CXX_STANDARD 17)

 if(MLIR_STANDALONE_BUILD)
   find_package(LLVM CONFIG REQUIRED)

@h-vetinari
Copy link
Contributor Author

Thanks for the quick handling!

@h-vetinari
Copy link
Contributor Author

Could we get a backport to release/16.x?

/cherry-pick 0096d17

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2023

Failed to cherry-pick: 0096d17

https://github.com/llvm/llvm-project/actions/runs/4120730621

Please manually backport the fix and push it to your github fork. Once this is done, please add a comment like this:

/branch <user>/<repo>/<branch>

@h-vetinari
Copy link
Contributor Author

/branch h-vetinari/llvm-project/mlir_cxx

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2023

/pull-request llvm/llvm-project-release-prs#284

@h-vetinari
Copy link
Contributor Author

Could we reopen this please until the backport is merged?

@EugeneZelenko EugeneZelenko reopened this Feb 9, 2023
tru pushed a commit that referenced this issue Feb 9, 2023
This is only useful when building the project in a "standalone" way: that is by
invoking cmake pointing at mlir/ to build against an already built LLVM.

Fixes #60574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants