-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
move providers to UMF #1
Conversation
/// This structure comprises function pointers used by corresponding | ||
/// umfMemoryProvider* calls. Each memory provider implementation should | ||
/// initialize all function pointers. | ||
struct umf_memory_provider_ops_t { | ||
/// Version of the ops structure. | ||
/// Should be initialized using UMF_VERSION_CURRENT | ||
uint32_t version; | ||
umf_memory_provider_type_t type; // TODO change to caps? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the provider_type should be a part of this struct. I think it might be better to just associate the type with a provider in the globaProviders
map. (e,g, std::map<std::string, std::pair<umf_memory_provider_ops_t, umf_memory_provider_type_t>> globalProviders;
)
This way, users do not need to worry about setting this type for their own providers.
Also, perhaps in future, we will have some providers that could be mapped to 2 different types. In that case, it would also be better to not keep the type inside the ops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, maybe we could make types into bit flags, so checking if a provider supports something would be as simple as:
if (provider->supported_types & memspace->type) {...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely removed the "type" from the provider ops and use support_device() instead
source/loader/ur_memory_provider.hpp
Outdated
|
||
#include "umf/memory_provider.h" | ||
|
||
struct ur_provider_config_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need memory type (shared/device/host). See here: https://github.com/intel/llvm/pull/10269/files in usm.hpp file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
3a4d29f
to
ec3b1c7
Compare
// TODO comment | ||
enum umf_result_t | ||
umfMemoryProviderRegister(struct umf_memory_provider_ops_t *ops); | ||
enum umf_result_t umfMemoryProvidersRegisterGetNames(char *providers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registry ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
return UMF_RESULT_SUCCESS; | ||
} | ||
|
||
enum umf_result_t umfMemoryProvidersRegisterGetNames(char *providers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think names are important. I'd create some opaque 'factory' type that'd return here in a simple array. This factory type would then have methods like ...get_name
or ...get_ops
.
Alternatively, you could move the selection of providers based on name or some other abstracted config here.
Returning everything in a string array just to parse it higher up the stack looks like an unnecessary complication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed using names and use simple array of ops instead
ec3b1c7
to
128bede
Compare
const umf_memory_provider_ops_t *umfMemoryProvidersRegistryGetOps(char *name) { | ||
auto it = std::find_if( | ||
std::begin(globalProviders), std::end(globalProviders), | ||
[&](auto &ops) { return std::strcmp(ops.get_name(NULL), name) == 0; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general comment: until now we assumed that all function pointers in ops
would only be used through the public APIs of memory pool or provider, not accessed directly. This means that, you could assume the first argument (NULL in your case) is never NULL. If that's no longer the case we should probably add some info about this in documentation (for get_name at least)
size_t *numProviders); | ||
|
||
const struct umf_memory_provider_ops_t * | ||
umfMemoryProvidersRegistryGetOps(char *name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be better to return enum umf_result_t
as in other APIs and return ops
as an output parameter?
@@ -18,6 +18,8 @@ | |||
extern "C" { | |||
#endif | |||
|
|||
typedef struct umf_memory_provider_t *umf_memory_provider_handle_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where it is used?
No description provided.