[NNPA] Add numerical test for NNPA #1335

imaihal · 2022-04-12T07:50:35Z

This PR adds numerical tests for NNPA.

Currently tests for MatMul2D, Gemm, LSTM, and GRU are provided and run by using following command.

cmake --build . --config Release --target check-onnx-numerical-nnpa

These tests uses the same test code with numerical tests for CPU (test/modellib and test/numerial), but uses different cmake file(test/accelerator/NNPA/numerical/CMakeLists.txt).

Gemm
Since alpha and beta should be one for Matmul of zDNN library, #ifdef directive TEST_GEMM_ALPHA_BETA_1 are added in test/numerical/TestGemm.cpp and set in the CMakeLists.txt (test/accelerator/NNPA/numerical/CMakeLists.txt)
LSTM
Since LSTM of zDNN library does not support peephole tensor, #ifdef directive TEST_LSTM_NONEP_ONLY are added in test/numerial/TestLSTM.cpp and set in the CMakeLists.
Currently bidirectinal LSTM is not supported in onnx-mlir for NNPA, so, it is disabled by using #ifdef directive TEST_RNN_NO_BIDIR
GRU
Since GRU of zDNN library does not support LinearBeforeReset, #ifdef directive TEST_GRU_L1 are added in test/numerial/TestGRU.cpp and set in the CMakeLists.
Currently bidirectinal LSTM is not supported in onnx-mlir for NNPA, so, it is disabled by using #ifdef directive TEST_RNN_NO_BIDIR

To use these #ifdef directives, this PR updated add_onnx_mlir_executable() in MLIR.cmake.

ATOL and RTOL for accuracy check
ATOL and RTOL need to be changed for tests. They are configured by environment variable TEST_ATOL and TES_RTOL in CMakeLists.txt.
Verification that zDNN instruction are generated in shared library
checkInstructionFromEnv() in ModelLibBuilder verifies that the instruction specified the environment variable is included in generated shared library. The instructions such as zdnn_matmul_op, zdnn_lstm, and zdnn_gru are specified in CMakeLists.txt.
Test results on a machine with NNPA (I removed the elapsed time because I don't know if I can publish it here.)

Test project /home/trl/imaihal/onnx-mlir/build/test/accelerators/NNPA/numerical
    Start 1: TestMatMul2DNNPA
1/4 Test #1: TestMatMul2DNNPA ................   Passed
    Start 2: TestGemmNNPA
2/4 Test #2: TestGemmNNPA ....................   Passed
    Start 3: TestLSTMNNPA
3/4 Test #3: TestLSTMNNPA ....................   Passed
    Start 4: TestGRUNNPA
4/4 Test #4: TestGRUNNPA .....................   Passed

100% tests passed, 0 tests failed out of 4

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

AlexandreEichenberger

The code changes are good. I would like for you to add a document page on AccelNNPAHowToUseAndTest.md and list the explanation that you gave on the conversation. Over time, this page will add more info.

Please make this addition part of this PR. Thanks

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2022-04-19T06:15:43Z

I will remove backend test from this PR because we need to discuss how to set ATOL and RTOL for backend test. We need to set them to pass backend test on NNPA.
Update: Created PR for backend test #1415

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2022-05-24T23:06:09Z

@jenkins-droid test this please

AlexandreEichenberger

Its going in the right directions, and the new infrastructure about testing for the ops is very helpful. Thanks

test/numerical/TestGRU.cpp

AlexandreEichenberger · 2022-05-25T02:19:42Z

test/modellib/ModelLib.cpp

+  dptr = (int *)dlsym(handle, instructionName.c_str());
+  if (dptr == NULL) {
+    printf("%s\n", dlerror());
+    return false;


If you look at the code in ExecutionSession builder, there is already code that can load a library without being specific to windows or linux.

_sharedLibraryHandle = llvm::sys::DynamicLibrary::getPermanentLibrary(sharedLibPath.c_str()); if (!_sharedLibraryHandle.isValid()) throw std::runtime_error(reportLibraryOpeningError(sharedLibPath));

The execution session also look for specific entries, so I would investigate if we can reuse the OS agnostic methods developed by LLVM.

IN particular, we also look there if a given function is found or not, for example

_queryEntryPointsFunc = reinterpret_cast<queryEntryPointsFuncType>( _sharedLibraryHandle.getAddressOfSymbol(_queryEntryPointsName.c_str())); if (!_queryEntryPointsFunc) throw std::runtime_error(reportSymbolLoadingError(_queryEntryPointsName));

test/numerical/TestGRU.cpp

test/numerical/TestGemm.cpp

test/numerical/TestLSTM.cpp

test/numerical/TestMatMul2D.cpp

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

This reverts commit 6bb232f. Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

AlexandreEichenberger · 2022-05-26T21:32:39Z

@imaihal let me know when you have all your changes in, and I will review again.

imaihal · 2022-05-27T01:05:31Z

@AlexandreEichenberger Yes. Now ready for review again. Thanks!

tungld

@tungld I created checkSharedLibInstruction() to verify instructions in shared library. We can verify that zDNN instructions are included automatically by using the function.

Thanks so much! It's useful.

The current code looks good to me with some minor changes to make things consistent. Please wait for Alex's input too.

In the future, we can improve the code with two things:

checkInstructionFromEnv should support multiple instructions since one ONNX op may call multiple zDNN ops.
gemm.compileAndLoad() && gemm.checkInstructionFromEnv(). compileAndLoad has loaded the shared library, no need to load it again in checkInstructionFromEnv. To do that, we should edit ExecutionSession so that we can get the loaded library from it.

tungld · 2022-05-27T02:47:15Z

test/modellib/ModelLib.cpp

+  std::string instructionName =
+      getenv("TEST_CHECK_INSTRUCTION") ? getenv("TEST_CHECK_INSTRUCTION") : "";
+  if (instructionName.empty())
+    return true;


This looks redundant since checkInstruction(instructionName) also checks empty string.

Please pass the env var name as a parameter. Its easier to read all the param in the specific test file, e.g. TestMatMul2D....

Done. Thanks!

tungld · 2022-05-27T03:00:35Z

test/numerical/TestGemm.cpp

@@ -92,10 +93,15 @@ int main(int argc, char *argv[]) {
      const auto aTrans = *rc::gen::inRange(0, 2);
      const auto bTrans = *rc::gen::inRange(0, 2);
      const auto cRank = *rc::gen::inRange(1, 3);
+#ifdef TEST_ALPHA_BETA_1


Suggest to use a more specific name, say TEST_GEMM_ALPHA_BETA_1.

Done. Thanks!

tungld · 2022-05-27T03:01:47Z

test/numerical/TestLSTM.cpp

@@ -70,14 +80,23 @@ int main(int argc, char *argv[]) {
    // Whether initial value of the cell(initial_c) is specified.
    const auto isNoneC = *rc::gen::element(0, 1);
    // Whether the weight tensor for peepholes(P) is specified.
+#ifdef TEST_NONEP_ONLY


Suggest to use TEST_LSTM_NO_P or TEST_LSTM_NO_PEEPHOLE to make it consistent with the other environment variables.

Done. Thanks!

tungld · 2022-05-27T03:04:38Z

test/accelerators/NNPA/numerical/CMakeLists.txt

+  LINK_LIBS PRIVATE ${TEST_LINK_LIBS}
+  )
+
+add_numerical_unittest(TestGemm-NNPA zdnn_matmul_op


Could we check multiple zdnn functions here? For example, some GEMM ops will use both zdnn_matmul_op and zdnn_add_op. It's ok to implement this with another PR.

Thanks for your comments!
It is possible to check multiple functions, but currently it is not easy to select one function from those multiple functions in each testcase.
For example in GEMM, zdnn_matmul_op and zdnn_add_op can be checked in all testcases of GEMM, but it is not possible to select only zdnn_matmul_op in some testcases and zdnn_matmul_op and zdnn_add_op in the other testcases. I think we need another config for each testcase.
So, I would like to set this TODO for now.

AlexandreEichenberger · 2022-05-27T16:16:09Z

gemm.compileAndLoad() && gemm.checkInstructionFromEnv(). compileAndLoad has loaded the shared library, no need to load it again in checkInstructionFromEnv. To do that, we should edit ExecutionSession so that we can get the loaded library from it.

definitely, just add

llvm::sys::DynamicLibrary &getDynLibraryHandle() { return _sharedLibraryHandle };

AlexandreEichenberger

Almost there... please implement the changes suggested by Tung and I. The added functionality of handing a list of instructions to check should probably let for a subsequent PR.

I will approve it now so that you may merge it once the above changes are made. Big thanks.

AlexandreEichenberger · 2022-05-27T16:18:00Z

test/modellib/ModelLib.cpp

+  std::string instructionName =
+      getenv("TEST_CHECK_INSTRUCTION") ? getenv("TEST_CHECK_INSTRUCTION") : "";
+  if (instructionName.empty())
+    return true;


Please pass the env var name as a parameter. Its easier to read all the param in the specific test file, e.g. TestMatMul2D....

AlexandreEichenberger · 2022-05-27T16:21:00Z

test/modellib/ModelLib.cpp

+  if (instructionName.empty())
+    return true;
+  std::string sharedLibName = getSharedLibName(sharedLibBaseName);
+  llvm::sys::DynamicLibrary sharedLibraryHandle =


Please extract the library from the ExecutionSession model variable onnx_mlir::ExecutionSession *exec;

Done. Thanks!

AlexandreEichenberger · 2022-05-27T16:21:40Z

test/numerical/TestConv.cpp

@@ -40,8 +40,9 @@ bool isOMConvTheSameAsNaiveImplFor(const int N, const int C, const int H,

  Conv2DLibBuilder conv(SHARED_LIB_BASE.str(), N, C, H, W, kH, kW, autoPad,
      pHBegin, pHEnd, pWBegin, pWEnd, stride, dilation, isDynamic);
-  return conv.build() && conv.compileAndLoad() && conv.prepareInputs() &&
-         conv.run() && conv.verifyOutputs();
+  return conv.build() && conv.compileAndLoad() &&


It looks SOOO much better, thanks for the changes.

…ction Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

jenkins-droid · 2022-05-30T10:19:13Z

Jenkins Linux ppc64le Build #5206 [push] [NNPA] Add numerical tes... started at 06:22

jenkins-droid · 2022-05-30T10:19:14Z

Jenkins Linux s390x Build #6084 [push] [NNPA] Add numerical tes... started at 06:19

jenkins-droid · 2022-05-30T10:19:17Z

Jenkins Linux amd64 Build #6069 [push] [NNPA] Add numerical tes... started at 05:19

jenkins-droid · 2022-05-30T11:19:33Z

Jenkins Linux s390x Build #6084 [push] [NNPA] Add numerical tes... passed after 1 hr 0 min

jenkins-droid · 2022-05-30T11:32:19Z

Jenkins Linux ppc64le Build #5206 [push] [NNPA] Add numerical tes... passed after 1 hr 13 min

jenkins-droid · 2022-05-30T11:33:49Z

Jenkins Linux amd64 Build #6069 [push] [NNPA] Add numerical tes... passed after 1 hr 14 min

imaihal added 13 commits April 8, 2022 03:27

Initial test for numrical and backend tests for nnpa

2bfeeca

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Fix setting for atol and rtol for numerical tests

86b0b86

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Create directories for backend tests for NNPA

779aaf9

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove code for backend test from test/backend

73021fa

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add numerical tests for NNPA

84b7ca8

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove numerical tests from main numerical tests

9060ad3

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

bba79db

Fix config for numerical tests of NNPA.

32b6db2

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove duplicated code for backend tests

3d3abc7

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove inference_backend_nnpa.py

9a2516e

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove TestLoop for NNPA

0c0823d

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Clean up CMakeLists in backend test

ffda9e6

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove inference_backend_nnpa in test_nnpa.py

4841a11

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal marked this pull request as ready for review April 13, 2022 08:45

imaihal added 2 commits April 14, 2022 04:09

Clean up makefile

1665e06

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

70b0db1

AlexandreEichenberger requested changes Apr 14, 2022

View reviewed changes

imaihal added 4 commits April 15, 2022 02:32

Merge branch 'main' into nnpa_tests

8fc0bd7

Remove test_nnpa.py

ba38b3b

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

0a35b38

Merge branch 'main' into nnpa_tests

850f69f

imaihal changed the title ~~Add backend and numerical test for NNPA~~ [NNPA] Add numerical test for NNPA Apr 19, 2022

imaihal added 6 commits April 19, 2022 07:10

Remove LD_PRELOAD

ae1799e

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

eed67bd

Merge branch 'main' into nnpa_tests

4ace29d

Merge branch 'main' into nnpa_tests

e345c5c

Added numerical tests of LSTM for NNPA

37915f9

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Remove backend test for NNPA

32f7557

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld marked this pull request as draft April 22, 2022 08:30

AlexandreEichenberger reviewed May 25, 2022

View reviewed changes

imaihal added 7 commits May 25, 2022 09:07

Use loop in all case in LSTM and GRU

4748544

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Refactor a function for checking instruction.

44ad4d8

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

e180925

Get symbol using llvm APIs

b76e69f

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Use checkInstructionFromEnv() in Conv and RNN

626a73e

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Test for failure: test for checkInstructionFromEnv()

6bb232f

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Revert "Test for failure: test for checkInstructionFromEnv()"

a0ee501

This reverts commit 6bb232f. Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal force-pushed the nnpa_tests branch from 060c67a to a0ee501 Compare May 26, 2022 07:26

Merge branch 'main' into nnpa_tests

86c5cae

tungld reviewed May 27, 2022

View reviewed changes

AlexandreEichenberger approved these changes May 27, 2022

View reviewed changes

imaihal added 6 commits May 30, 2022 02:04

Merge branch 'main' into nnpa_tests

adb7122

Change interface of checkInstructionFromEnv() and env name for instru…

008167a

…ction Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add getSharedLibraryHandle() in ExecutionSession

c7c3430

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into nnpa_tests

d622e5e

Add TODO

353beb7

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Update docs

edc4ce9

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal merged commit 042031f into onnx:main May 30, 2022

imaihal deleted the nnpa_tests branch May 30, 2022 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NNPA] Add numerical test for NNPA #1335

[NNPA] Add numerical test for NNPA #1335

imaihal commented Apr 12, 2022 •

edited

Loading

AlexandreEichenberger left a comment

imaihal commented Apr 19, 2022 •

edited

Loading

imaihal commented May 24, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger May 25, 2022 •

edited

Loading

AlexandreEichenberger commented May 26, 2022

imaihal commented May 27, 2022

tungld left a comment

tungld May 27, 2022

AlexandreEichenberger May 27, 2022

imaihal May 30, 2022

tungld May 27, 2022

imaihal May 30, 2022

tungld May 27, 2022

imaihal May 30, 2022

tungld May 27, 2022

imaihal May 30, 2022

AlexandreEichenberger commented May 27, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger May 27, 2022

AlexandreEichenberger May 27, 2022

imaihal May 30, 2022

AlexandreEichenberger May 27, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

[NNPA] Add numerical test for NNPA #1335

[NNPA] Add numerical test for NNPA #1335

Conversation

imaihal commented Apr 12, 2022 • edited Loading

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

imaihal commented Apr 19, 2022 • edited Loading

imaihal commented May 24, 2022

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

AlexandreEichenberger May 25, 2022 • edited Loading

Choose a reason for hiding this comment

AlexandreEichenberger commented May 26, 2022

imaihal commented May 27, 2022

tungld 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

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

AlexandreEichenberger commented May 27, 2022

AlexandreEichenberger 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

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

jenkins-droid commented May 30, 2022

imaihal commented Apr 12, 2022 •

edited

Loading

imaihal commented Apr 19, 2022 •

edited

Loading

AlexandreEichenberger May 25, 2022 •

edited

Loading