[BLAS] Fix symbol conflicts between backends and reference #251

dnhsieh-intel · 2022-11-18T02:37:15Z

Description

When a backend respects CBLAS symbols, instead of comparing the backend against the reference libcblas library, the current testing structure would compare the reference against itself due to the conflicting symbols. This PR resolves this issue by loading reference functions at runtime into a local namespace.

Fixes #204

Tests

Diagnostic messages were inserted in the Netlib cblas_saxpy and the oneMKL axpy unit test to check when the reference was called. More specifically:

In the Netlib cblas_saxpy.c

void cblas_saxpy( const CBLAS_INT N, const float alpha, const float *X,
                       const CBLAS_INT incX, float *Y, const CBLAS_INT incY)
{
   printf("   Netlib cblas_saxpy called\n");
#ifdef F77_INT
   F77_INT F77_N=N, F77_incX=incX, F77_incY=incY;
#else
   #define F77_N N
   #define F77_incX incX
   #define F77_incY incY
#endif
   F77_saxpy( &F77_N, &alpha, X, &F77_incX, Y, &F77_incY);
}

In the oneMKL unit test axpy.cpp

template <typename fp>
int test(device *dev, oneapi::mkl::layout layout, int N, int incx, int incy, fp alpha) {
    // Prepare data.
    vector<fp> x, y, y_ref;

    rand_vector(x, N, incx);
    rand_vector(y, N, incy);
    y_ref = y;

    // Call Reference AXPY.
    std::cout << "Call reference axpy" << std::endl;
    using fp_ref = typename ref_type_info<fp>::type;
    const int N_ref = N, incx_ref = incx, incy_ref = incy;

    ::axpy(&N_ref, (fp_ref *)&alpha, (fp_ref *)x.data(), &incx_ref, (fp_ref *)y_ref.data(),
           &incy_ref);

    // Call DPC++ AXPY.
    std::cout << "Call DPC++ axpy" << std::endl;
    ......

We use the _ct test as an example in the following. The results of the _rt test are the same.

Since the conflicts of symbols between the MKL backend and the reference have been fixed in PR #210, the reference function was called appropriately both with and without this PR:

$ ./bin/test_main_blas_ct --gtest_filter=AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
Run this program with --terse_output to change the way it prints its output.
Note: Google Test filter = AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AxpyTestSuite/AxpyTests
[ RUN      ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
[       OK ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz (125 ms)
[----------] 1 test from AxpyTestSuite/AxpyTests (125 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (125 ms total)
[  PASSED  ] 1 test.

To simulate the situation of conflicting symbols, the commit 634d7d2 right before PR #210 was used. Based on the commit, the diagnostic messages were:

$ ./bin/test_main_blas_ct --gtest_filter=AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
Run this program with --terse_output to change the way it prints its output.
Note: Google Test filter = AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AxpyTestSuite/AxpyTests
[ RUN      ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
   Netlib cblas_saxpy called
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
   Netlib cblas_saxpy called
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
   Netlib cblas_saxpy called
[       OK ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz (104 ms)
[----------] 1 test from AxpyTestSuite/AxpyTests (104 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (104 ms total)
[  PASSED  ] 1 test.

We then applied the changes in this PR to the commit. We can see from the messages that the changes fixed the issue.

$ ./bin/test_main_blas_ct --gtest_filter=AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
Run this program with --terse_output to change the way it prints its output.
Note: Google Test filter = AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major*CPU*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AxpyTestSuite/AxpyTests
[ RUN      ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
Call reference axpy
   Netlib cblas_saxpy called
Call DPC++ axpy
[       OK ] AxpyTestSuite/AxpyTests.RealSinglePrecision/Column_Major_Intel_R__Core_TM__i7_6770HQ_CPU___2_60GHz (116 ms)
[----------] 1 test from AxpyTestSuite/AxpyTests (116 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (117 ms total)
[  PASSED  ] 1 test.

Checklist

All Submissions

Do all unit tests pass locally? Attach a log: blas_lnx_log.txt
Have you formatted the code using clang-format?

andrewtbarker

Thanks for this pull request and the thorough investigation! I have a question about this:

To simulate the situation of conflicting symbols, the commit 634d7d2 right before PR #210 was used.

Why this particular commit? The problem of using reference blas for both baseline and test persists even after #210.

andrewtbarker · 2022-11-18T15:50:22Z

tests/unit_tests/blas/include/reference_blas_templates.hpp

+            izamax_res = cblas_izamax_p(n, x, incx);
+    }
+    return izamax_res;
+}


This file is already huge, I suggest putting everything above this point in a separate file (maybe named reference_blas_wrappers.hpp or something similar) and including it in reference_blas_templates.hpp. I actually wish there was a way to avoid so much wrapping and indirection but probably this is the best we can do.

The problem of using reference blas for both baseline and test persists even after #210.

That's weird. I didn't see the problem in the develop using either gdb or output messages. Could you share what you observed please?

This open source product calls oneMKL CPU backend via DPC++ interfaces, which then call cblas symbols. I recall that those symbols were resolving to netlib even after #210. This is what I meant with this comment. But you have investigated this more closely than I did, I might be wrong.

I think we might have dodged the conflict. After PR #210, it looks to me that it is the _64 APIs that are called in the mklcpu backend, e.g., cblas_saxpy_64.

The wrappers were moved to reference_blas_wrappers.hpp. (commit f17ce6c)

Oh, that makes sense, the _64 wrappers were a separate change. Thanks!

mkrainiuk

Thanks for the PR! I have several questions to the changes.

tests/unit_tests/CMakeLists.txt

mkrainiuk · 2022-11-18T19:12:52Z

tests/unit_tests/CMakeLists.txt

      PROPERTIES TEST_PREFIX ${DOMAIN_PREFIX}/RT/
      DISCOVERY_TIMEOUT 30
    )
  endif()

  gtest_discover_tests(test_main_${domain}_ct
    PROPERTIES BUILD_RPATH ${CMAKE_BINARY_DIR}/lib
-    PROPERTIES ENVIRONMENT LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/lib:$ENV{LD_LIBRARY_PATH}
+    PROPERTIES ENVIRONMENT LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/lib:${CBLAS_LIB_DIR}:$ENV{LD_LIBRARY_PATH}


Please add domain check here if possible

Addressed in the commit f762e6f.

Thank you! I have one more question regarding Windows build, we define LD_LIBRARY_PATH here for Netlib libraires but we don't define PATH on Windows, does it make sense to update PATH instead of LD_LIBRARY_PATH in case of Windows?

I thought this is straightforward. I was wrong...

On Windows, if we try to set PATH, which is a semicolon-separated list, in gtest_discover_tests, for example:

gtest_discover_tests(test_main_${domain}_ct PROPERTIES ENVIRONMENT "PATH=C:\path1;C:\path2;C:\path3")

After CMake's internal processing, it turns out that the actual setting will only be PATH=C:\path1. This seems to be a known CMake issue.

Do you think we should open an issue to investigate a workaround? Right now, because users have to include Netlib in PATH to build on Windows, it's fine if they run tests directly after building. But they would encounter the "failed to load CBLAS library" error if they run tests in a separate session.

I think it's worth adding a note to our README about Netlib required in PATH on Windows. And we also can investigate potential W/A as separated issue. I don't think we need to mix the current issue and the Windows one in this PR.

Will be tracked in issue #252.

tests/unit_tests/CMakeLists.txt

tests/unit_tests/blas/include/reference_blas_templates.hpp

dnhsieh-intel · 2022-11-21T03:23:11Z

Log of the commit f17ce6c: blas_lnx_log_f17ce6c.txt
100% tests passed, 0 tests failed out of 3530

mkrainiuk

The changes look good to me. Thank you!

dnhsieh-intel added 2 commits November 17, 2022 10:44

Update CMake files for CBLAS reference

8b53b0d

Add wrappers for CBLAS reference

cb0f053

dnhsieh-intel requested review from mkrainiuk and andrewtbarker November 18, 2022 02:38

dnhsieh-intel self-assigned this Nov 18, 2022

andrewtbarker reviewed Nov 18, 2022

View reviewed changes

mkrainiuk reviewed Nov 18, 2022

View reviewed changes

dnhsieh-intel added 2 commits November 20, 2022 17:30

Restrict changes in cmake files to BLAS domain

f762e6f

Separate BLAS reference wrappers

f17ce6c

andrewtbarker approved these changes Nov 21, 2022

View reviewed changes

mkrainiuk approved these changes Nov 21, 2022

View reviewed changes

dnhsieh-intel mentioned this pull request Nov 22, 2022

[BLAS][LAPACK] Add Netlib in PATH for Windows testing #252

Open

dnhsieh-intel merged commit 205179e into uxlfoundation:develop Nov 22, 2022

dnhsieh-intel deleted the symbols_clash branch December 14, 2022 22:09

mmeterel mentioned this pull request Apr 13, 2023

Netlib BLAS functions don't work correctly #271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BLAS] Fix symbol conflicts between backends and reference #251

[BLAS] Fix symbol conflicts between backends and reference #251

dnhsieh-intel commented Nov 18, 2022

andrewtbarker left a comment

andrewtbarker Nov 18, 2022

dnhsieh-intel Nov 18, 2022

andrewtbarker Nov 18, 2022

dnhsieh-intel Nov 21, 2022

andrewtbarker Nov 21, 2022

mkrainiuk left a comment •

edited

Loading

mkrainiuk Nov 18, 2022

dnhsieh-intel Nov 21, 2022

mkrainiuk Nov 21, 2022

dnhsieh-intel Nov 22, 2022

mkrainiuk Nov 22, 2022

dnhsieh-intel Nov 22, 2022

dnhsieh-intel commented Nov 21, 2022

mkrainiuk left a comment

[BLAS] Fix symbol conflicts between backends and reference #251

[BLAS] Fix symbol conflicts between backends and reference #251

Conversation

dnhsieh-intel commented Nov 18, 2022

Description

Tests

Checklist

All Submissions

andrewtbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkrainiuk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhsieh-intel commented Nov 21, 2022

mkrainiuk left a comment

Choose a reason for hiding this comment

mkrainiuk left a comment •

edited

Loading