intel / libvpl

Intel® Video Processing Library (Intel® VPL) API, dispatcher, and examples
https://intel.github.io/libvpl/
MIT License
262 stars 80 forks source link

Code expects __ILP32__/__LP64__ to be defined, missing on ARM #70

Closed StefanBruens closed 1 year ago

StefanBruens commented 1 year ago

ILP32 is not defined on 32bit ARM (although it is an ILP32 arch): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106930

The following code fails to build: https://github.com/oneapi-src/oneVPL/blob/master/api/vpl/mfxdefs.h#L62-L76

One workaround is to add -D__ILP32__ to CXXFLAGS.

jeffreymcallister commented 1 year ago

Thank you for reporting this issue. We are looking into options to resolve and will let you know as we make progress.

StefanBruens commented 1 year ago

Unfortunately, no response from GCC folks yet.

As any GCC change would only affect future GCC versions, a workaround in oneVPL is probably the way to go:

#elif defined(__ILP32__) || defined(__arm__)

jonrecker commented 1 year ago

Should be resolved by the commit: https://github.com/oneapi-src/oneVPL/commit/6182082a12a4fc90b3db89568b3924f2c88f2704

StefanBruens commented 1 year ago

Should be resolved by the commit: 6182082

Unfortunately not. From configuration summary:

[   16s] -- ---------------- Configuration summary ------------------------------
[   16s] -- CMake:
[   16s] --   CMAKE_VERSION                   : 3.25.1
[   16s] --   CMAKE_GENERATOR                 : Unix Makefiles
[   16s] --   CMAKE_BUILD_TOOL                : /usr/bin/gmake
[   16s] -- Target:
[   16s] --   CMAKE_SYSTEM_NAME               : Linux
[   16s] --   CMAKE_SYSTEM_VERSION            : 6.1.4-1-default
[   16s] --   CMAKE_SYSTEM_PROCESSOR          : armv7l
[   16s] -- General:
[   16s] --   CMAKE_BUILD_TYPE                : RelWithDebInfo
[   16s] --   CMAKE_TOOLCHAIN_FILE            : 
[   16s] --   CMAKE_C_COMPILER                : /usr/bin/cc
[   16s] --   CMAKE_CXX_COMPILER              : /usr/bin/c++
[   16s] --   Build architecture              : 32-bit
StefanBruens commented 1 year ago

@jonrecker - BTW, cherry-picking commits from another persons MR and submitting these under your own name is quite bad style.

jonrecker commented 1 year ago

Thank you, and apologies for merging changes into one commit. We had issues getting suitable build fixes staged properly in the staging repo which this repository mirrors. The workaround here checks whether CMAKE_CXX_COMPILER contains arm-linux. Additional checks to enable this workaround may be added at that point in CMakeLists.txt. Feel free to create a PR if you think that could be a solution and we will merge it. Perhaps CMAKE_SYSTEM_PROCESSOR?

Unfortunately editing api/vpl/mfxdefs.h directly is not possible here. These headers are considered read-only in this repository as they are taken from the upstream specification here. The specification is updated less often so changes will take time to propagate.

StefanBruens commented 1 year ago

Unfortunately editing api/vpl/mfxdefs.h directly is not possible here. These headers are considered read-only in this repository as they are taken from the upstream specification here. The specification is updated less often so changes will take time to propagate.

Then this should be fixed in the oneAPI-spec tree. For one, the workaround here is broken and fragile. Second, adding a -D__ILP32__ workaround in every downstream user of mfxdefs.h is not feasible.

StefanBruens commented 1 year ago

https://github.com/oneapi-src/oneAPI-spec/pull/468

jonrecker commented 1 year ago

Agreed, updating the headers is the best long-term solution. Thank you for creating a PR into the spec repository. Please note that these headers are usually updated ~ a few times a year to correspond with API version updates. So it may be some time before the change is available. However since this is not an API change but a fix to support a new compiler, possibly the headers can be updated sooner. I will inquire.

Can you confirm that __arm__ will be defined only for 32-bit builds with all ARM compilers? And are any similar changes required for 64-bit builds (__aarch64__)?

mav-intel commented 1 year ago

Fixed in 6e9f56a