Onednn: non-gemm_driver extended_sgemm --> wrong results

[kruus]

It is possible that extended_sgemm invoke cblas_sgemm or ref_gemm with
unsupported (integer/packed) formats, leading to test failures.
Incorrect extended_sgemm implementations can occur in 2 cases:

1. compile with USE_CBLAS --> cblas_sgemm
2. or mayiuse(sse41) is false, so ref_gemm<float> is used.

A poor solution might be to fail primitive creation in one of the incompletely handled cases.
I would prefer that alternate code paths detect and handle the inappropriate extended_sgemm calls with a correct 'C' impl. This could be via a non-JIT gemm_driver call, or maybe some appropriate ref_gemm<T> call or some other specialty reference impl.

---

Environment

mkl-dnn v1.0

Steps to reproduce

Either compiled with USE_CBLAS or modify extended_sgemm to push
everything through ref_gemm<float> instead of gemm_driver or run on some
very old x86 machine [I guess].

Actual behavior

Tests fail. Wrong results frequently occur for 'Packed' [integer] formats when extended_sgemm chooses to invoke cblas_sgemm or ref_gemm<float> instead of gemm_driver

Expected behavior

Tests pass

Original [longer] outputs in #415

[mgouicem]

Hi @kruus .
These two issues are separate. The item 1. should be addressed as part of the #514 .

Regarding item number 2, we should be returning unimplemented as we have absolutely no support for those as of now.
It can be fixed by applying the following patch:
```c++
diff --git a/src/cpu/gemm/f32/ref_gemm_f32.cpp b/src/cpu/gemm/f32/ref_gemm_f32.cpp
index 183486b..b103b15 100644
--- a/src/cpu/gemm/f32/ref_gemm_f32.cpp
+++ b/src/cpu/gemm/f32/ref_gemm_f32.cpp
@@ -185,6 +185,10 @@ mkldnn_status_t ref_gemm(
const int *lda_, const data_t *B, const int *ldb_, const data_t *beta_,
data_t *C, const int *ldc_, const data_t *bias) {

• if (!( utils::one_of(*transa_, 'n', 'N', 't', 'T')
• && utils::one_of(*transb_, 'n', 'N', 't', 'T')))

• return mkldnn_unimplemented;
  +
  bool isTransA = (transa_ == 'T' || *transa_ == 't');
  bool isTransB = (transb_ == 'T' || transb_ == 't');
  const int M = *M_, N = *N_, K = *K_;
  diff --git a/src/cpu/gemm/s8x8s32/ref_gemm_s8x8s32.cpp b/src/cpu/gemm/s8x8s32/ref_gemm_s8x8s32.cpp
  index ae3ca7f..9c53886 100644
  --- a/src/cpu/gemm/s8x8s32/ref_gemm_s8x8s32.cpp
  +++ b/src/cpu/gemm/s8x8s32/ref_gemm_s8x8s32.cpp
  @@ -39,6 +39,10 @@ mkldnn_status_t ref_gemm_s8x8s32(const char *transa, const char *transb,
  if (M == 0 || *N == 0 || *K == 0)
  return mkldnn_success;

• if (!( utils::one_of(*transa, 'n', 'N', 't', 'T')

• && utils::one_of(*transb, 'n', 'N', 't', 'T')))
• return mkldnn_unimplemented;
  +
  bool OCisR = (offsetc == 'R' || *offsetc == 'r');
  bool OCisC = (offsetc == 'C' || offsetc == 'c');
  bool AisN = (transa == 'N' || *transa == 'n');

``

[kruus]

Still a tiny crack in USE_CBLAS case, after applying #514 and this patch.

• Tested with USE_CBLAS [and without SSE] so ref_gemm<float> is a backup method.
• Only one test fails

45: [ RUN      ] TestGEMM_fp32/gemm_test.TestGEMM/19
45: [  SKIPPED ] CPU does not support non-zero offsets.
45: [       OK ] TestGEMM_fp32/gemm_test.TestGEMM/19 (0 ms)
45: [ RUN      ] TestGEMM_fp32/gemm_test.TestGEMM/20
45: [  SKIPPED ] CPU does not support non-zero offsets.
45: [       OK ] TestGEMM_fp32/gemm_test.TestGEMM/20 (0 ms)
45: [ RUN      ] TestGEMM_fp32/gemm_test.TestGEMM/21
45: /local/kruus/mkl-dnn/tests/gtests/test_gemm_common.hpp:445: Failure
45: e evaluates to 3.8084843158721924,

--> 97% tests passed, 1 tests failed out of 37

• Tested without USE_CBLAS and with ref_gemm always (gemm_driver disabled), test 45:../21+ are all UNIMPL:

45: [ RUN      ] TestGEMM_packed_fp32/gemm_test.TestGEMM/21
45: ref_gemm<float>(P,t;MNK=2000,2000,2000;alpha=1.000000,A@ld=2000,B@ld=2000,beta=0.000000,C@ld=2000,bias)
45: [  UNIMPL  ] Implementation not found
45: [       OK ] TestGEMM_packed_fp32/gemm_test.TestGEMM/21 (116 ms)

---> "100% test passed, 0 tests failed out of 37"

• Not sure what condition ref_gemm is catching that USE_CBLAS still needs to check.

[kruus]

Trying if (!force_jit_nocopy_gemm && !(packed_A || packed_B)) { instead of #514 suggestion...

[emfomenk]

Isn't
!force_jit_nocopy_gemm && !(packed_A || packed_B)
the same as
!(force_jit_nocopy_gemm || packed_A || packed_B)?

[mgouicem]

This should be completely equivalent:
!( A || B) <-> !A && !B.
Here I suspect something else is going on.

[kruus]

duh, same. I'm ready for the weekend.

...
45: [  FAILED  ] TestGEMM_fp32/gemm_test.TestGEMM/21, where GetParam() = 104-byte object <6E-6E 00-00 00-00 00-00 D0-07 00-00 00-00 00-00 D0-07 00-00 00-00 00-00 D0-07 00-00 00-00 00-00 00-00 80-3F 00-00 00-00 D0-07 00-00 00-00 00-00 D0-07 00-00 00-00 00-00 D0-07 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00> (617 ms)
45: [ RUN      ] TestGEMM_fp32/gemm_test.TestGEMM/22

[mgouicem]

So I ran the test and with the patches applied, it will end up with a call to cblas_sgemm (it will not fallback to ref_gemm) as inputs are not packed, and force_no_copy = false.

So here I suspect that the issue might be in the library you are using for cblas functions

[kruus]

Got identical wrong result from

• Blas from intel-mkl-64bit-2018.2-046 and
• "Fortran 77 reference implementation".

So I tried with export OMP_NUM_THREADS=1, and everything is correct.

Maybe it's yet another case of omp library mismatch on x86. I'm going to use ref_gemm on x86, and only USE_CBLAS on my VE Aurora platform (only a single openmp available).

Your patches completely fix the packed-format bugs, @mgouicem. Many thanks.

[mgouicem]

Glad to hear that the patches fixed your issues.
Yes it seems to be an issue with mixing OMP runtimes.
I build MKL-DNN with gcc 4.8.3. I ran it with MKL 2019.4 on an AVX2 system (Xeon E5 E5-2699) and OMP_NUM_THREADS=22 and it indeed fails. However, if I force load libiomp5 by using LD_PRELOAD=libiomp5.so the test passes.

If you can use MKL for cblas calls, you have 3 options to not face this omp runtime issue:
1) use clang compiler
2) explicitly link to libiomp5.so (using LD_PRELOAD for example)
3) set the MKL_THREADING_LAYER=GNU environment variable to make MKL use the GNU OMP runtime.

The two first options are preferred since libiomp generally provides better performance.
Hope this helps.