Multi-threading support refactor

[ericxu233]

First stage of the multi-threading support refactor by providing a wrapper mutex class around the current routines and use std::loack_guard() to enable RAII practices.

[ericxu233]

Note: Using std::lock_guard() since std::scope_lock() requires the mutex to do std::try_lock() which our underlying implementation doesn't support.

[ericxu233]

Preliminary hardware testing done. Now doing bulk change.

[ericxu233]

Klockwork is failing for some reason...not sure why...

174 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/test/acl_program_test.cpp:394 ABV.STACK (1:Critical) Analyze
Array 'this->m_context->device' of size 128 may use index value(s) 0..MAX
  * acl_program_test.cpp:394: 'm_context' is passed as argument '$1->num_devices' to function 'clCreateProgramWithBuiltInKernels'.
  * acl_program_test.cpp:394: Index value(s) 0..MAX may be used to access array 'this->m_context->device' of size 128 while calling 'clCreateProgramWithBuiltInKernels'.
    * acl_program_test.cpp:394: Result of expression '($1-><id>_cl_context::num_devices</id>)+(-1)' is >= 0.
      * acl_program_test.cpp:394: Function argument '$1->num_devices' is >= 1.
    * acl_program_test.cpp:394: Value(s) >= 0 is used as violation index in function 'clCreateProgramWithBuiltInKernels'.
      * acl_program.cpp:719: 'context' is passed as argument '$1->num_devices' to function 'clCreateProgramWithBuiltInKernelsIntelFPGA'.
        * acl_program.cpp:636: 'context' is passed as argument '$1->num_devices' to function 'acl_context_uses_device'.
          * acl_context.cpp:454: Array 'context->device' of size 128 is used.
Current status 'Analyze'

Summary: 1 Local
1 Total Issue(s)

@zibaiwan @pcolberg Have any ideas on this issue? I did not modify the code highlighted by klockwork...

[pcolberg]

@zibaiwan @pcolberg Have any ideas on this issue? I did not modify the code highlighted by klockwork...

This is a tricky one. Have you tried adding debug printfs to the existing lock functions,

fpga-runtime-for-opencl/src/acl_thread.cpp

Lines 23 to 37 in 412bdf9

	void acl_lock() {
	l_init_once();
	if (acl_global_lock_count == 0) {
	acl_acquire_condvar(&l_acl_global_condvar);
	}
	acl_global_lock_count++;
	}
	void acl_unlock() {
	acl_assert_locked();
	acl_global_lock_count--;
	if (acl_global_lock_count == 0) {
	acl_release_condvar(&l_acl_global_condvar);
	}
	}

as well as the new lock functions,

fpga-runtime-for-opencl/src/acl_thread.cpp

Lines 77 to 79 in 412bdf9

	void aclMutex::lock() { acl_lock(); }
	void aclMutex::unlock() { acl_unlock(); }

and running the test in question to see whether the locking behaviour differs from before the refactoring?

export AOCL_BOARD_PACKAGE_ROOT=$(realpath test/board/a10_ref)
./acl_test -v -g acl_program -n create_from_builtin

[ericxu233]

On a unrelated note to the previous issue: acl_condition_lock_guard will not be templated for now since I can't seem to get it work. Will add improvements later.

[ericxu233]

Indeed found that the locking and locking are different...but maybe due to another thing (I didn't refactor acl_test yet). Will do more testings later...

[ericxu233]

The klockwork issue is not related to locking. Confirmed that the locking pattern is the same with the test failing.
Although, there is the new code change has these extra outputs for some reason.

[ericxu233]

clCreateProgramWithBuiltInKernels(m_context, 1, &m_device[0], "hello", &status);
The klockwork issue is not recognizing the second argument 1 as passing? There is a risk in clCreateProgramWithBuiltInKernels since the num_devices argument (the second argument) is a 32 bit integer but context->devices only is a 128 size array. This makes this portion of the function very risky:

[ericxu233]

clCreateProgramWithBuiltInKernels(m_context, 1, &m_device[0], "hello", &status); The klockwork issue is not recognizing the second argument 1 as passing? There is a risk in clCreateProgramWithBuiltInKernels since the num_devices argument (the second argument) is a 32 bit integer but context->devices only is a 128 size array. This makes this portion of the function very risky:

Changing the function arguments from 1 to 0 fixed the klockwork issue...very weird. (This breaks the unit test tho)

[zibaiwan]

Klocwork seemed to have passed with the latest fix.

[ericxu233]

Testing done. No runtime errors introduced with sycl-l3 test runs.

[zibaiwan]

Testing done. No runtime errors introduced with sycl-l3 test runs.

@ericxu233 , thanks for doing the testing. If not already done, please run the ocl test as well. You can find the test set in the official FullCycle-oneAPI-ocl-weekly-lnx run. If it doesn't include the multithreading test that we discussed earlier, please run the multi-threading test separately as well.

[zibaiwan]

Please run clang-format by following the instruction in contributing. I know the name of the workflow "run clang-format" sounds like it may run the clang-format for you, but it actually only checks if your code is in clang-format and doesn't modify your pull request. (You probably already know this, just in case you don't.).

[zibaiwan]

The change looks good to me as long as the conversations are resolved and the additional tests are completed.

[ericxu233]

@pcolberg Could you please take a look and see if the changes are ok :)

[pcolberg]

Thanks @ericxu233, this is close to completion 🙂

[zibaiwan]

Thanks @ericxu233 . Looks good to me!

[pcolberg]

Thanks Eric, this is ready for merging 🙂

I have slightly updated the comments in acl_thread.h and removed left-over lock_count in acl_event.cpp and acl_mem.cpp which were causing warnings.

I also moved acl_is_locked_callback(), acl_wait_for_device_update(), and acl_signal_device_update() back to their old location. You had moved them to be consistent with the order in acl_thread.h, but I think it's fine to have the order in acl_thread.cpp be slightly out of sync, in favour of modifying as little code as possible. That will be appreciated by a future reader, e.g., looking at git blame.

Please let me know if everything is in order before this is merged.

[pcolberg]

Third time is the charm 🙂

You can omit the mutex type when constructing the scoped lock:

diff --git a/src/acl_command.cpp b/src/acl_command.cpp
index 6e71fc4..23a8c12 100644
--- a/src/acl_command.cpp
+++ b/src/acl_command.cpp
@@ -38,7 +38,7 @@
 ACL_EXPORT
 CL_API_ENTRY cl_int CL_API_CALL
 clEnqueueBarrierIntelFPGA(cl_command_queue command_queue) {
-  std::scoped_lock<acl_mutex_wrapper_t> lock{acl_mutex_wrapper};
+  std::scoped_lock lock{acl_mutex_wrapper};
 
   if (!acl_command_queue_is_valid(command_queue)) {
     return CL_INVALID_COMMAND_QUEUE;

[ericxu233]

@pcolberg Looks good to me. I think everything is in order now. We are ready to merge. Thanks for the detailed code review and explanatoins!