intel / intel-graphics-compiler

Other
597 stars 155 forks source link

External SPIRV-Tools #219

Closed ArchangeGabriel closed 2 years ago

ArchangeGabriel commented 2 years ago

Latest released switched IGC_BUILD__SPIRV_TOOLS_ENABLED to be default ON on all platforms. I could not find a way to use a prebuilt spirv-tools, is there any? If not, please consider allowing it and in the mean time I’d like to know what is lost if I switch back to disabling this.

ArchangeGabriel commented 2 years ago

(By disabling I mean patching set(IGC_BUILD__SPIRV_TOOLS_ENABLED ON) in https://github.com/intel/intel-graphics-compiler/blob/master/IGC/CMakeLists.txt to OFF)

eero-t commented 2 years ago

Regarding SPIRV tooling needed by IGC in general, I could not get IGC compiled any more without providing it also SPIRV-Headers / SPIRV-Tools / SPIRV-LLVM-Translator sources. Additionally, IGC itself wants to checkout specific commits from last one, so it needs to be fairly full Git repo (not "--depth 1"). Looking at the IGC release notes, opencl-clang and IGC VC are build against different SPIRV-LLVM-Translator commits, so I assume there's some good reason for this.

ArchangeGabriel commented 2 years ago

@eero-t I’m still able to compile without SPIRV-LLVM-Translator sources (but using prebuilts with upstream tagged releases).

eero-t commented 2 years ago

IGC does not build with the latest SPIRV-Tools "v2022.1" release sources:

[63/982] Building CXX object IGC/Release/external/SPIRV-Tools/build/source/CMakeFiles/SPIRV-Tools-static.dir/val/validate_image.cpp.o
FAILED: IGC/Release/external/SPIRV-Tools/build/source/CMakeFiles/SPIRV-Tools-static.dir/val/validate_image.cpp.o 
/usr/bin/c++ -DCL_KHR_FP64_EXT -DGHAL3D=USC -DICBE_LINUX -DIGC_CMAKE -DIGC_EXPORTS=1 -DIGC_SPIRV_ENABLED -DIGC_VC_ENABLED -DINSIDE_PLUGIN -DISTDLIB_UMD -DLINUX -DLLVM_VERSION_MAJOR=11 -DNDEBUG -DNOMINMAX -DSPIRV_COLOR_TERMINAL -DSPIRV_LINUX -DSPIRV_TIMER_ENABLED -DSTD_CALL -DUSC_EXPORTS=1 -DUSE_MMX -DUSE_SSE -DUSE_SSE2 -DUSE_SSE3 -DUSE_SSSE3 -D_AMD64_ -D_COMPILER_DLL_ -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_IGC_ -D_SCL_SECURE_NO_WARNINGS -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -IIGC/LLD_INCLUDE_DIR-NOTFOUND/../.. -IIGC/WrapperLLVM/include -IIGC/autogen -IIGC -IIGC/common -IIGC/.. -IIGC/../Common -IIGC/../3d/common -IIGC/../inc -IIGC/../inc/common -IIGC/../inc/common/Compiler -IIGC/../inc/common/Compiler/API -IIGC/../visa/include -IIGC/Release -IIGC/AdaptorOCL/ocl_igc_shared/executable_format -IIGC/AdaptorOCL -IIGC/AdaptorOCL/ocl_igc_interface/impl -IIGC/../inc/common/Compiler/common -IIGC/AdaptorOCL/cif/cif/.. -IIGC/ZEBinWriter/zebin/source -IIGC/ZEBinWriter/zebin/source/autogen -I/home/nobody/source/SPIRV-Tools -I/home/nobody/source/SPIRV-Tools/include -IIGC/Release/external/SPIRV-Tools/build -Iexternal/SPIRV-Tools/../../../SPIRV-Headers/include -isystem /usr/lib/llvm-11/include -fno-exceptions -fdata-sections -ffunction-sections -O2 -pipe -fmessage-length=0 -march=corei7 -mstackrealign -fms-extensions -Werror -Wno-unused-parameter -Wno-missing-field-initializers -Wwrite-strings -Wno-long-long -Wswitch -Wno-sign-compare -Wno-unused-result -Wno-enum-compare -Wno-type-limits -Wno-ignored-qualifiers -Wno-shadow -Wformat -Wformat-security -Wno-extra -Wno-write-strings -finline -fno-strict-aliasing -msse -msse2 -msse3 -mssse3 -msse4 -msse4.1 -msse4.2 -Wno-unknown-pragmas -fPIC -Bsymbolic -D_FORTIFY_SOURCE=2 -fstack-protector -finline-functions -funswitch-loops -Wno-maybe-uninitialized -lrt -fno-rtti -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG -g -fPIC -fno-rtti -Wall -Wextra -Wnon-virtual-dtor -Wno-missing-field-initializers -Werror -std=c++11 -fno-exceptions -Wno-long-long -Wshadow -Wundef -Wconversion -Wno-sign-conversion -std=gnu++11 -MD -MT IGC/Release/external/SPIRV-Tools/build/source/CMakeFiles/SPIRV-Tools-static.dir/val/validate_image.cpp.o -MF IGC/Release/external/SPIRV-Tools/build/source/CMakeFiles/SPIRV-Tools-static.dir/val/validate_image.cpp.o.d -o IGC/Release/external/SPIRV-Tools/build/source/CMakeFiles/SPIRV-Tools-static.dir/val/validate_image.cpp.o -c /home/nobody/source/SPIRV-Tools/source/val/validate_image.cpp
/home/nobody/source/SPIRV-Tools/source/val/validate_image.cpp: In function 'bool spvtools::val::{anonymous}::CheckAllImageOperandsHandled()':
/home/nobody/source/SPIRV-Tools/source/val/validate_image.cpp:75:10: error: 'SpvImageOperandsNontemporalMask' was not declared in this scope; did you mean 'SpvImageOperandsMinLodMask'?
   75 |     case SpvImageOperandsNontemporalMask:
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |          SpvImageOperandsMinLodMask
/home/nobody/source/SPIRV-Tools/source/val/validate_image.cpp: In function 'spv_result_t spvtools::val::{anonymous}::ValidateImageOperands(spvtools::val::ValidationState_t&, const spvtools::val::Instruction*, const spvtools::val::{anonymous}::ImageTypeInfo&, uint32_t)':
/home/nobody/source/SPIRV-Tools/source/val/validate_image.cpp:635:14: error: 'SpvImageOperandsNontemporalMask' was not declared in this scope; did you mean 'SpvImageOperandsMinLodMask'?
  635 |   if (mask & SpvImageOperandsNontemporalMask) {
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |              SpvImageOperandsMinLodMask

Building succeeds only after downgrading SPIRV-Tools sources to "v2021.4" release.

tjaalton commented 2 years ago

still no resolution to this after nearly three months.. trying to keep this stack updated in a distro is a hopeless task

pszymich commented 2 years ago

Hi, I wanted to address two separate issues we have here:

  1. Build problem with SPIRV-Tools 2022.1 I managed to reproduce the problem using

    • SPIRV-Tools revision v2022.1
    • SPIRV-Headers revision sdk-1.2.198.0

    Pulling SPIRV-Headers to HEAD of master branch fixed the problem. The issue is only for SPIRV-Tools-static build target. This tells me SPIRV-Headers latest release is not compatible with v2022.1 SPIRV-Tools release. The commit in SPIRV-Headers that fixes the issue.

  2. Adding support for system installed SPIRV-Tools/Headers libraries We are currently exploring how to prioritize the task of adding support for these preinstalled libraries. We've raised the priority of it and should have some progress information next week.

eero-t commented 2 years ago

There's a SPIRV-Headers bug for getting new release that would support latest SPIRV-Tools: https://github.com/KhronosGroup/SPIRV-Headers/issues/266 (containing link to corresponding bug in SPIRV-Tools).

pszymich commented 2 years ago

@agrachiv, please update with an ETA. Thanks!

agrachiv commented 2 years ago

Hi! I hope to will fix it until the end of the work week.

eero-t commented 2 years ago

I’m still able to compile without SPIRV-LLVM-Translator sources (but using prebuilts with upstream tagged releases).

@ArchangeGabriel Btw. IGC builds fine with the sources from the new "sdk-1.3.204.0" tagged SPIRV headers and tools projects, in case you want to test updating the external deps.

(I'm still having issue #224 with compute-runtime builds when using newer IGC releases though.)

pszymich commented 2 years ago

@agrachiv can you provide an updated ETA? Thanks

agrachiv commented 2 years ago

Hi! I am really sorry for doing it for so long. For now, I've committed into our source repo, this commit should arrive here soon.

ArchangeGabriel commented 2 years ago

Is that why https://github.com/intel/intel-graphics-compiler/pull/230 was closed? It has been merged internally instead?

frantisekz commented 2 years ago

@agrachiv , to be able to use Fedora's system spirv-tools, I had to make this change: #234 , can you take a look at it if it makes sense?

Thanks!

ArchangeGabriel commented 2 years ago

@frantisekz Have you tried #230?

frantisekz commented 2 years ago

@frantisekz Have you tried #230?

Yep, the PR I've created is based on master (which has #230 ). I've tried building the igc with 230 and then 234 applied on the source.

ArchangeGabriel commented 2 years ago

(Right, I missed the fact that #230 was merged as https://github.com/intel/intel-graphics-compiler/commit/d790a8f8571c51aeccac1a320b4fcf7ed71a2a26)

agrachiv commented 2 years ago

@agrachiv , to be able to use Fedora's system spirv-tools, I had to make this change: #234 , can you take a look at it if it makes sense?

Thanks!

Hi! I've checked it out, everything looks all right. I've committed this change into our repo, it will take some time to arrive here.

frantisekz commented 2 years ago

@agrachiv , fyi, it seems the 7e6b78d1ccf5afb4080795aa87029e55b9e5499c pulled the #234 only partially (the -static needs to be removed on two lines to make it work).

ArchangeGabriel commented 2 years ago

I can confirm that the current trunk + the missing part of #234 works. While trying that, I’ve spotted some typos, so I’ve PR’ed #235 with the remaining part of #234 + fixes for those.

agrachiv commented 2 years ago

Hi, @ArchangeGabriel! Sorry for the delay. I've already merged it, should arrive here soon.

ArchangeGabriel commented 2 years ago

Indeed, I see that you authored acfa99b5aefbebf037e4c939f5a8e592dfaed04e. Not part of the latest release, but should be part of the next one, but that’s the only patch to backport. All-in-all, these have been amazing progress in IGC lastly, so that we can now build fully from external copies. :)

pszymich commented 2 years ago

Thanks @agrachiv, @ArchangeGabriel!

Closing for now, please reopen or resubmit a ticket if any issues arise.