AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.09k stars 357 forks source link

Support LLVM-18 #1769

Closed MehdiChinoune closed 6 months ago

MehdiChinoune commented 7 months ago

Problem

OSL fails to build against LLVM-18 (RC2) which is to be released soon. I tried to build it by modifying src/cmake/externalpackages.cmake to accept LLVM-18, but It requires more changes.

--- a/src/cmake/externalpackages.cmake
+++ b/src/cmake/externalpackages.cmake
@@ -117,7 +117,6 @@
 # LLVM library setup
 checked_find_package (LLVM REQUIRED
                       VERSION_MIN 9.0
-                      VERSION_MAX 17.9
                       PRINT LLVM_SYSTEM_LIBRARIES CLANG_LIBRARIES)
 # ensure include directory is added (in case of non-standard locations
 include_directories (BEFORE SYSTEM "${LLVM_INCLUDES}")

Expected behavior:

To build successfully

Actual behavior:

Fails to build

Steps to Reproduce

  1. Install/Build LLVM-18
  2. Build against it

Versions

lgritz commented 7 months ago

I don't suppose you could make a PR to implement this fix?

MehdiChinoune commented 7 months ago

I don't suppose you could make a PR to implement this fix?

It requires knowldge of OSL source code.

lgritz commented 7 months ago

No, I just meant a PR that implements the one-line patch you propose to the cmake files to reject llvm 18.

MehdiChinoune commented 7 months ago

I have a larger patch

--- a/src/cmake/externalpackages.cmake
+++ b/src/cmake/externalpackages.cmake
@@ -117,7 +117,7 @@
 # LLVM library setup
 checked_find_package (LLVM REQUIRED
                       VERSION_MIN 9.0
-                      VERSION_MAX 17.9
+                      VERSION_MAX 18.9
                       PRINT LLVM_SYSTEM_LIBRARIES CLANG_LIBRARIES)
 # ensure include directory is added (in case of non-standard locations
 include_directories (BEFORE SYSTEM "${LLVM_INCLUDES}")
--- a/src/liboslcomp/oslcomp.cpp
+++ b/src/liboslcomp/oslcomp.cpp
@@ -26,7 +26,11 @@
 #include <clang/Frontend/TextDiagnosticPrinter.h>
 #include <clang/Frontend/Utils.h>
 #include <clang/Lex/PreprocessorOptions.h>
-#include <llvm/Support/Host.h>
+#if OSL_LLVM_VERSION < 160
+#    include <llvm/Support/Host.h>
+#else
+#    include <llvm/TargetParser/Host.h>
+#endif
 #include <llvm/Support/MemoryBuffer.h>
 #include <llvm/Support/ToolOutputFile.h>
 #include <llvm/Support/raw_ostream.h>
--- a/src/liboslexec/llvm_passes.h
+++ b/src/liboslexec/llvm_passes.h
@@ -349,7 +349,11 @@
                                         // Should handle promoting whatever the constant value is (most likely zeroinitializer)
                                         llvm::ConstantFolder Folder;
                                         auto* signExtConstant
+#if OSL_LLVM_VERSION < 180
                                             = Folder.CreateCast(
+#else
+                                            = Folder.FoldCast(
+#endif
                                                 llvm::Instruction::SExt,
                                                 constant, m_native_mask_type);

--- a/src/liboslexec/llvm_util.cpp
+++ b/src/liboslexec/llvm_util.cpp
@@ -45,7 +45,11 @@
 #include <llvm/Support/CommandLine.h>
 #include <llvm/Support/ErrorOr.h>
 #include <llvm/Support/FileSystem.h>
-#include <llvm/Support/Host.h>
+#if OSL_LLVM_VERSION < 160
+#    include <llvm/Support/Host.h>
+#else
+#    include <llvm/TargetParser/Host.h>
+#endif
 #include <llvm/Support/raw_os_ostream.h>
 #if OSL_LLVM_VERSION < 140
 #    include <llvm/Support/TargetRegistry.h>

There still some Type::getInt*PtrTy that should be either chnaged to PointerType::getUnqual or PointerType::get (like in https://github.com/llvm/llvm-project/commit/7b9d73c2f90) depending in the case, that's where I am stuck. I will see if I could clone OSL and open a PR with these changes

MehdiChinoune commented 7 months ago

Sorry to close, but I solved the issue for myself by building against LLVM-15

lgritz commented 7 months ago

Reopening. We DO need to support LLVM 18 regardless of whether you in particular need it.

MehdiChinoune commented 6 months ago

Please, open a new separate issue.