Onednn: compat_libs does not look correct when building shared lib

[pinzhenx]

Similar to https://github.com/oneapi-src/oneDNN/issues/743

In Linux, if we are building oneDNN as shared lib, it will generate a dead symlink libmkldnn.a that points to nowhere.

If we are building shared lib in windows, it will mistakenly copy the non-existing bin/dnnl.lib to bin/mkldnn.lib, but actually dnnl.lib was in /lib

[emfomenk]

More info or, better, a reproducer? :)
I start thinking we need an option to disable the building of compat libs -- it seems it will solve the #768 as well.

[pinzhenx]

For issue on windows, it's reported from pytorch community https://github.com/pytorch/pytorch/issues/42115

I think if we can have such an option, we don't have to reproduce this problem at all.

[emfomenk]

Still having a standalone reproducer would be nice to have. Otherwise it is unclear whom to blame :) Maybe this is due to the way oneDNN is integrated to pyTorch.

[xkszltl]

Regarding option to disable compat lib....Emmm IA64 is right there.

Well just another Intel joke, don't take that serious.
I think if it's just a bad path it won't be that hard to get it right, right?

[xkszltl]

If work-around option is provided, I think it's better to have a manually specified path instead.

[emfomenk]

Hi @xkszltl,

I think if it's just a bad path it won't be that hard to get it right, right?

I agree that fixing the issue is preferred over just work-arounding it with an extra build option.
And I would love to fix this, but I need a small reproducer to work with -- figuring out how oneDNN is integrated to pyTorch build system was not very pleasant task last time I did that while investigating #743.

W/o reproducer I will look into pyTorch myself but that would just take more time.

If work-around option is provided, I think it's better to have a manually specified path instead.

Could you please elaborate? Do you suggest to have an option that indicates the path where the compat lib should be put?

[xkszltl]

This is how I corrected the wrong part without disabling it:
https://github.com/pytorch/pytorch/issues/42115#issuecomment-665756365

The copy failed because both from/to paths are wrong.
Somehow when build with pytorch, cmake think the main file of that target is in bin, and I force set it to lib.
I mean instead of a kill switch, having an option to overwrite this path would be the "minimum fix".

[xkszltl]

This is the build script I'm using:
https://github.com/xkszltl/Roaster/blob/master/win/pkgs/pytorch.ps1

It's not a minimum repro (because several other scripts are required to run in advance to install dependencies), but those cmake options should be helpful when creating a minimum repro.

[vpirogov]

The Windows part of this issue is resolved by #806. Do we still need to figure out what's happening on Linux?

[pinzhenx]

@CaoZhongZ

[emfomenk]

If libdnnl.so is built in a non-standard path, that libmkldnn.so could point to non-existing dependency (by default, it just points to ./libdnnl.so that is located somewhere else). In turn this makes the install fail, as copying the invalid symlink leads to cp failure (unless -P option is passed, that cmake doesn't do).

@pinzhenx, is this the behavior you observe?
If so, could you please check the following patch for Linux?

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 5a16687c1..6aecb68b2 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -182,7 +182,11 @@ else()
             COMMAND ${CMAKE_COMMAND} -E create_symlink libdnnl${ext_and_ver} ${compat_link}
             DEPENDS ${LIB_NAME})
         add_custom_target(compat_libs${ver} ALL DEPENDS ${compat_link})
-        install(FILES ${compat_link} DESTINATION ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
+        set(install_compat_link "${compat_link}")
+        if(NOT IS_ABSOLUTE "${install_compat_link}")
+            set(install_compat_link "${CMAKE_CURRENT_BINARY_DIR}/${install_compat_link}")
+        endif()
+        install(FILES ${install_compat_link} DESTINATION ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
     endforeach()
 endif()

[pinzhenx]

Unfortunately, this patch doesn't work.

The issue on linux is when we are building static lib, the libmkldnn.a should not go to the lib_location, which we override with the value of LIBRARY_OUTPUT_DIRECTORY, instead, it still lives in third_party/oneDNN/src/

[CaoZhongZ]

How about append create symlink command in target "dnnl" to generate libmkldnn.so.. The downside of this approach is if some one delete the libmkldnn, it won't regenerate itself if you type 'make'.

[emfomenk]

Unfortunately, this patch doesn't work.

@pinzhenx, is there a way to reproduce the issue on our side? What cmake flags you used to confirm the issue still persists?

How about append create symlink command in target "dnnl" to generate libmkldnn.so.

@CaoZhongZ, Not sure I understand. dnnl is not a standalone target the we can extend, as it is added automatically with add_library() command. But even if we could, again, I don't understand what this approach really fixes as we don't have a reproducer showing the problem in the first place.

Please provide one to us :)

[emfomenk]

BTW, just for the record: I confirmed the issue by adding set(CMAKE_LIBRARY_OUTPUT_DIRECTORY custom_dir) in the main CMakeLists.txt file. The patch above at least solves the problem for this scenario.

[CaoZhongZ]

It's may or may not be oneDNN's problem. I'm looking into it. 🤔️

[emfomenk]

It's may or may not be oneDNN's problem. I'm looking into it.

Well, at least set(CMAKE_LIBRARY_OUTPUT_DIRECTORY custom_dir) shows that oneDNN has a problem :)
But anyways, thanks! Looking forward to your findings.

[CaoZhongZ]

Found it, missed CMAKE_ARCHIVE_OUTPUT_DIRECTORY in extension building system. 🤦‍♂️

[CaoZhongZ]

@CaoZhongZ, Not sure I understand. dnnl is not a standalone target the we can extend, as it is added automatically with add_library() command. But even if we could, again, I don't understand what this approach really fixes as we don't have a reproducer showing the problem in the first place.

Please provide one to us :)

There is lazy solution concept like:

add_custom_command(TARGET ${LIB_NAME}
    COMMAND ${CMAKE_COMMAND} -E create_symlink libdnnl${ext_and_ver} $<TARGET_LINKER_FILE_DIR:${LIB_NAME}>/${compat_link}
    POST_BUILD ...)

It'll generate the symbolic link where it should be, and no duplication. However this solution do not prevent some handsy ones when they only remove the libmkldnn.* links. It won't regenerate.

But anyway current solution works great, less modification less problem. 😄

[pinzhenx]

Previously we've set LIBRARY_OUTPUT_DIRECTORY unconditionally but without the ARCHIVE_OUTPUT_DIRECTORY. Ideally, it should have something like this. But anyway, we could handle it in the integration.

        if(DNNL_LIBRARY_TYPE STREQUAL "SHARED")
            get_property(lib_location TARGET ${LIB_NAME} PROPERTY LIBRARY_OUTPUT_DIRECTORY)
        else()
            get_property(lib_location TARGET ${LIB_NAME} PROPERTY ARCHIVE_OUTPUT_DIRECTORY)
        endif()