Skip to content

Cache context offline device setting specified by environment variables (2nd stage) #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Cache context offline device setting specified by environment variables (2nd stage) #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions include/acl_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,8 @@ void acl_reset(void);
// if it is not.
cl_bool acl_init_from_hal_discovery(void);

// Looks at environment variable CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA.
// If it exists and is prefixed by "+" then:
// Return a pointer to the device name (without the "+" prefix).
// Set *use_offline_ret_only = 1
// If it exists and is not prefixed by "+" then
// Return a pointer to the device name.
// Set *use_offline_ret_only = 0
#define ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY 0
#define ACL_CONTEXT_OFFLINE_ONLY 1
#define ACL_CONTEXT_MSIM 3
#define ACL_CONTEXT_MPSIM 4
const char *acl_get_offline_device_user_setting(int *use_offline_only_ret);
acl_context_offline_mode_t
acl_get_offline_device_user_setting(std::string *offline_device);

ACL_EXPORT
extern struct _cl_platform_id acl_platform;
Expand Down
28 changes: 23 additions & 5 deletions include/acl_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,20 @@ typedef enum {
ACL_COMPILER_MODE_NUM_MODES = CL_CONTEXT_COMPILER_MODE_NUM_MODES_INTELFPGA
} acl_compiler_mode_t;

// Looks at environment variable CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA.
// If it exists and is prefixed by "+" then:
// Return a pointer to the device name (without the "+" prefix).
// Set *use_offline_ret_only = 1
// If it exists and is not prefixed by "+" then
// Return a pointer to the device name.
// Set *use_offline_ret_only = 0
Comment on lines +219 to +225
Copy link
Contributor

@pcolberg pcolberg Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not document the possible modes, but the offline device name passed to acl_get_offline_device_user_setting(). Could you move this comment to the function body and replace the existing, duplicate comment? (There are two possible places for the comment; I prefer the function definition since that avoids having to jump between the comment and the implementation.)

typedef enum {
ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY,
ACL_CONTEXT_OFFLINE_ONLY,
ACL_CONTEXT_MSIM,
ACL_CONTEXT_MPSIM
} acl_context_offline_mode_t;
Comment on lines +226 to +231
Copy link
Contributor

@pcolberg pcolberg Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can introduce a scoped enum to group the constants:

Suggested change
typedef enum {
ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY,
ACL_CONTEXT_OFFLINE_ONLY,
ACL_CONTEXT_MSIM,
ACL_CONTEXT_MPSIM
} acl_context_offline_mode_t;
enum class acl_context_offline_mode {
offline_and_autodiscovery,
offline_only,
msim,
mpsim
};

Lowercase is easier to discern for the human reader. (Other projects use SnakeCase for similar reasons.)


/* When a feature is still in development, it might need enum values
* that are distinct from all published enums in the OpenCL token registry.
* There is a reserved range for values for such "experimental" features.
Expand Down Expand Up @@ -1226,7 +1240,8 @@ typedef struct _cl_device_id {

unsigned int address_bits; // cache address bits to avoid GetDeviceInfo calls

int present; // Is the device present in the host system?
bool present; // Is the device present in the host system?
bool offline; // Is the device a real (i.e., not simulator) offline device?

// Error notification callback.
CL_EXCEPTION_TYPE_INTEL device_exception_status;
Expand Down Expand Up @@ -1573,13 +1588,16 @@ typedef struct _cl_platform_id
int device_exception_platform_counter; // indicates number of devices with at
// least one exception

// The setting of environment variable CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA, if
// any.
std::string offline_device;
// Record whether the platform has (non-simulator) offline device,
// value will be:
// 1: if there is a valid offline device specified by environment variable
// 0: if there is no offline device specified by environment variable
// -1: if the offline device specified by environment variable is invalid
int has_offline_device;
// Cache context offline mode specified by environment variables
// CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA, CL_CONTEXT_MPSIM_DEVICE_INTELFPGA
// or CL_CONTEXT_MSIM_DEVICE_INTELFPGA
int offline_mode;
acl_context_offline_mode_t offline_mode;

// Should we track and automatically release leaked objects?
// This helps immensely with the OpenCL conformance tests which tend to
Expand Down
30 changes: 6 additions & 24 deletions src/acl_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ static void
l_init_kernel_invocation_wrapper(acl_kernel_invocation_wrapper_t *wrapper,
unsigned i);
static void l_forcibly_release_allocations(cl_context context);
static cl_device_id l_find_device_by_name(const std::string &name);
static cl_int l_update_program_library_root(cl_context context,
const char *new_root);
static cl_int l_update_compile_command(cl_context context, const char *new_cmd);
Expand Down Expand Up @@ -606,12 +605,11 @@ static cl_int l_load_properties(cl_context context,
}
}

// Environment variable can specify we always an offline device.
if (!acl_platform.offline_device.empty()) {
if (!l_find_device_by_name(acl_platform.offline_device))
ERR_RET(CL_INVALID_VALUE, context,
"Invalid offline device specified by environment variable "
"CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA");
// Check if environment variable specified offline device is valid
if (acl_platform.has_offline_device < 0) {
ERR_RET(CL_INVALID_VALUE, context,
"Invalid offline device specified by environment variable "
"CL_CONTEXT_OFFLINE_DEVICE_INTELFPGA");
}

// Get default for program_library_root.
Expand Down Expand Up @@ -817,17 +815,6 @@ static cl_int l_load_properties(cl_context context,
return CL_SUCCESS;
}

static cl_device_id l_find_device_by_name(const std::string &name) {
acl_assert_locked();

for (unsigned i = 0; i < acl_platform.num_devices; ++i) {
if (name == acl_platform.device[i].def.autodiscovery_def.name) {
return &(acl_platform.device[i]);
}
}
return 0;
}

// Initialize the given context.
// Yes, this is like a "placement new".
//
Expand Down Expand Up @@ -883,8 +870,6 @@ static cl_int l_init_context_with_devices(cl_context context,
int num_present = 0;
int num_absent = 0;
for (cl_uint i = 0; i < num_devices; i++) {
int usable = devices[i]->present;

// Can't mix both (actually) present and absent devices because there
// is no consistent way to place device global memory.
if (devices[i]->present) {
Expand All @@ -900,10 +885,7 @@ static cl_int l_init_context_with_devices(cl_context context,
"Can't create a context with both offline and online devices");
}

usable = usable || acl_platform.offline_device ==
devices[i]->def.autodiscovery_def.name;

if (!usable)
if (!devices[i]->present && !devices[i]->offline)
ERR_RET(CL_DEVICE_NOT_AVAILABLE, context, "Device not available");

// Mark the device(s) as opened
Expand Down
23 changes: 16 additions & 7 deletions src/acl_globals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,10 @@ static void l_reset_present_board() {
// If it's prefixed by "+", then it's in addition to any auto-discovered
// devices.
// If not, then we don't even probe for auto-discovered devices.
const char *acl_get_offline_device_user_setting(int *use_offline_only_ret) {
int use_offline_only = 0;
acl_context_offline_mode_t
acl_get_offline_device_user_setting(std::string *offline_device) {
Comment on lines +76 to +77
Copy link
Contributor

@pcolberg pcolberg Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about combining these into a struct to avoid the null pointer?

struct acl_context_offline_device {
  std::string name;
  acl_context_offline_mode mode;
}

acl_context_offline_mode_t use_offline_only =
ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY;
const char *setting = 0;
const char *setting_deprecated = 0;
const char *result = 0;
Expand Down Expand Up @@ -136,8 +138,15 @@ const char *acl_get_offline_device_user_setting(int *use_offline_only_ret) {
}
}

*use_offline_only_ret = use_offline_only;
return result;
if (offline_device) {
if (use_offline_only == ACL_CONTEXT_MPSIM) {
*offline_device = ACL_MPSIM_DEVICE_NAME;
} else if (result) {
*offline_device = result;
}
}
Comment on lines +141 to +147
Copy link
Contributor

@pcolberg pcolberg Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this function is indeed the right place for this code. However, could you replace the earlier setting = "1" (which is confusing since that does not appear to be an actually used device name) in favour of directly setting the device name to ACL_MPSIM_DEVICE_NAME?

What is the content of CL_CONTEXT_MSIM_DEVICE_INTELFPGA? Is that a boolean or an actual device name? I could not find an equivalent ACL_MSIM_DEVICE_NAME. Is the code for the old simulator still needed at all?


return use_offline_only;
}

int acl_init(const acl_system_def_t *newsys) {
Expand Down Expand Up @@ -166,11 +175,11 @@ int acl_init(const acl_system_def_t *newsys) {
// This function returns CL_TRUE if a hal is initialized and CL_FALSE
// if it is not.
cl_bool acl_init_from_hal_discovery(void) {
int use_offline_only = 0;
const acl_hal_t *board_hal;
acl_assert_locked();

(void)acl_get_offline_device_user_setting(&use_offline_only);
acl_context_offline_mode_t use_offline_only =
acl_get_offline_device_user_setting(NULL);

// Two jobs:
// 1. Set the HAL from the linked-in HAL library.
Expand Down Expand Up @@ -223,8 +232,8 @@ void acl_reset(void) {

l_reset_present_board();

acl_platform.offline_device = "";
acl_platform.offline_mode = ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY;
acl_platform.has_offline_device = 0;
acl_platform.num_devices = 0;
for (unsigned i = 0; i < ACL_MAX_DEVICE; ++i) {
acl_platform.device[i] = _cl_device_id();
Expand Down
4 changes: 2 additions & 2 deletions src/acl_hal_mmd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,6 @@ acl_mmd_get_system_definition(acl_system_def_t *sys,
buf[MAX_BOARD_NAMES_LEN]; // This is a bit subtle, pointers to device
// names might get cached by various routines
char *ptr, *saveptr;
int use_offline_only;

#ifdef _WIN32
// We're really relying on this being called before anything else
Expand Down Expand Up @@ -1248,7 +1247,8 @@ acl_mmd_get_system_definition(acl_system_def_t *sys,
#endif

// Dynamically load board mmd & symbols
(void)acl_get_offline_device_user_setting(&use_offline_only);
acl_context_offline_mode_t use_offline_only =
acl_get_offline_device_user_setting(NULL);
if (use_offline_only == ACL_CONTEXT_MPSIM) {

// Substitute the simulator MMD layer.
Expand Down
Loading