Perform a memory copy for simulation buffer with buffer location

[sophimao]

As simulator does not have any global memory interface information before reprogram, change #104 initializes the runtime device def to have ACL_MAX_GLOBAL_MEM global memories, each with the same global memory address range obtained from a predefined default autodiscovery string (see #104 for details).

This becomes an issue when a buffer is created with the buffer location property specifying a global memory whose address range beginning lies beyond the range defined in the default autodiscovery string, and is written before the device reprogram. The write will send the data to address range specified by the default autodiscovery string, however, the kernel will check for the data in the actual address range specified by the device specification file (ipinterfaces.xml) and find nothing in that location.

This change tries to detect the above situation, and force a memory copy from the incorrect address range to the actual global memory address range before launching the kernel, so that correct results can be ensured.

[pcolberg]

Thanks @sophimao! This change needs a unit test. You can check the existing tests to see if there is anything that could be used as a starting point.

[sophimao]

This change needs a unit test. You can check the existing tests to see if there is anything that could be used as a starting point.

Sorry can you elaborate more on this? I believe if we want to test the simulation flow we would need to replace the unit test environment variable from CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA to CL_CONTEXT_MPSIM_DEVICE_INTELFPGA? The two options are

1. do it in the setup of a test group or
2. make a context with the CL_CONTEXT_MPSIM_DEVICE_INTELFPGA

I think we probably have to go with the former since creating a context with the property doesn't make other runtime constructs aware that we are running simulation flow. Or is there another way we can do this without having to involve the CL_CONTEXT_* environment variables?

[pcolberg]

I believe if we want to test the simulation flow we would need to replace the unit test environment variable from CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA to CL_CONTEXT_MPSIM_DEVICE_INTELFPGA? The two options are

1. do it in the `setup` of a test group or

2. make a context with the `CL_CONTEXT_MPSIM_DEVICE_INTELFPGA`

I think we probably have to go with the former since creating a context with the property doesn't make other runtime constructs aware that we are running simulation flow. Or is there another way we can do this without having to involve the CL_CONTEXT_* environment variables?

Ideally the unit test directly invokes acl_realloc_buffer_for_simulator() to check if it has the desired effect. So you would need to set up the global data structures used by that function. Does that sound doable, or do you see fundamental blockers? If the latter, then maybe it needs to be broken up into smaller parts that are not (that) dependent on global data structures, such that you could reasonably unit test them independently.

Regarding environment variables, setenv() is not thread-safe and should never be used in multi-threaded programs. (Unfortunately the test suite uses it, which might become an issue if a background thread were introduced.) So, it would seem preferable to create a context with CL_CONTEXT_MPSIM_DEVICE_INTELFPGA, if the other parts of the runtime that do need the environment variable are not involved in the unit test.

[sophimao]

Ideally the unit test directly invokes acl_realloc_buffer_for_simulator() to check if it has the desired effect. So you would need to set up the global data structures used by that function. Does that sound doable, or do you see fundamental blockers? If the latter, then maybe it needs to be broken up into smaller parts that are not (that) dependent on global data structures, such that you could reasonably unit test them independently.

Regarding environment variables, setenv() is not thread-safe and should never be used in multi-threaded programs. (Unfortunately the test suite uses it, which might become an issue if a background thread were introduced.) So, it would seem preferable to create a context with CL_CONTEXT_MPSIM_DEVICE_INTELFPGA, if the other parts of the runtime that do need the environment variable are not involved in the unit test.

Thanks Peter! I managed to add a unit test by faking a device with multiple global memories. Let me know if there is anything else I should address 😊

[pcolberg]

Thanks @sophimao, looks good with the unit test! After addressing the minor comments, could you squash everything into a single commit? Then this is ready to be merged 🙂

[pcolberg]

Thanks @sophimao! Please go ahead with merging once the CI has passed.