Shouldn't this be 0?

So we could do that, but this will necessitate other changes in the code to produce an equivalent result as before. I think if we do this, I have to rename this variable in order to reflect its purpose better (and differentiate it from numBackends). See the proposed changes below:

modified   src/api/unified/symbol_manager.cpp
@@ -171,7 +171,7 @@ AFSymbolManager::AFSymbolManager()
     : activeHandle(nullptr)
     , defaultHandle(nullptr)
     , numBackends(0)
-    , numBackendHandles(NUM_BACKENDS)
+    , numCustomHandles(0)
     , backendsAvailable(0)
     , logger(loggerFactory("unified")) {
     // In order of priority.
@@ -239,7 +239,7 @@ af_err AFSymbolManager::setBackend(af::Backend bknd) {
 }
 
 af_err AFSymbolManager::addBackendLibrary(const char *lib_path) {
-    if ((numBackendHandles + 1) > MAX_BKND_HANDLES) {
+    if ((NUM_BACKENDS + numCustomHandles + 1) > MAX_BKND_HANDLES) {
         // No more space for an additional handle
         UNIFIED_ERROR_LOAD_LIB();
     }
@@ -266,8 +266,8 @@ af_err AFSymbolManager::addBackendLibrary(const char *lib_path) {
 
         if (show_load_path) { printf("Using %s\n", lib_path); }
 
-        bkndHandles[numBackendHandles] = handle;
-        numBackendHandles++;
+        bkndHandles[NUM_BACKENDS + numCustomHandles] = handle;
+        numCustomHandles++;
 
         return AF_SUCCESS;
     }

I think the new approach is easy to understand what the earlier variable numBackendHandles's purpose is.

However, I believe we can still retain old logic if we just rename numBackendHandles to customBackendHandleIndex or customBackendTracker or something like that.

Edit: I misunderstood @9prady9's comment about retaining the old logic. By that he meant keeping numBackendHandles (or the variable we're renaming it into) initialized to NUM_BACKENDS. So in any case, the explanation below is for @umar456 and anyone who's wondering about this line.

Thanks. However, the indexing into the bkndHandles array have to be adjusted if we initialize numBackendHandles or numCustomHandles to 0. Originally, the purpose of numBackendHandles is to count how many handles does bkndHandles contain. I initialized it to 3 to reserve space for the default 3 backend handles (regardless of whether the machine or the build actually supports all 3 backends), because of the way the AFSymbolManager constructor loads the libraries and indexes into bkndHandles:

/*** af/defines.h ***/
// These enums should be 2^x
typedef enum {
    AF_BACKEND_DEFAULT = 0,  ///< (Mark: don't mind the following comment, it is something we need to correct) Default backend order: OpenCL -> CUDA -> CPU
    AF_BACKEND_CPU     = 1,  ///< CPU a.k.a sequential algorithms
    AF_BACKEND_CUDA    = 2,  ///< CUDA Compute Backend
    AF_BACKEND_OPENCL  = 4   ///< OpenCL Compute Backend
} af_backend;

/*** symbol_manager.cpp ***/
    // In order of priority.
    // (Mark: this is the actual order that is implemented, not the comment above in the header)
    static const af_backend order[] = {AF_BACKEND_CUDA, AF_BACKEND_OPENCL,
                                       AF_BACKEND_CPU};

    // Decremeting loop. The last successful backend loaded will be the most
    // prefered one.
    for (int i = NUM_BACKENDS - 1; i >= 0; i--) {
        int backend          = order[i] >> 1;  // 2 4 1 -> 1 2 0
        bkndHandles[backend] = openDynLibrary(order[i]);
        if (bkndHandles[backend]) {
            activeHandle  = bkndHandles[backend];
            activeBackend = (af_backend)order[i];
            numBackends++;
            backendsAvailable += order[i];
        }
    }

We can see that the index backend is calculated based on the enum values of the backends. Notice that backend is the same no matter how many backends are actually available. Let's take the worst case scenario: suppose that only OpenCL is available. AF_BACKEND_OPENCL is 4, and shifting it left by 1 makes it 2, and so bkndHandles[2] now contains the handle for the libafopencl library. That means any custom libraries that will be added must take bkndHandles[3] and beyond. NUM_BACKENDS is 3 and that's why I initialized numBackendHandles to NUM_BACKENDS (bkndHandles is using numBackendHandles as its index in af_add_backend_library and af_set_backend_library).

Just value initialize this in the member initializer list to initialize bkndHandles.

see: https://stackoverflow.com/a/23987926/523374

Thanks, something new I learned.

Prefer ${ArrayFire_BINARY_DIR}

Thanks, that's clearer/more specific. I didn't know this variable existed.

Yeah, most PROJECT_* variables in CMake have <ProjectNameGivenInProjectCommand>_* variables.

These variables seem fragile in case we decide to put the binaries somewhere else. Perhaps we could pass arguments to the executable instead of hard coding it in the code. That way the build will tell the exe where to look which seems more appropriate.

I would suggest passing these as definitions via CMake, library suffix values are already available as variables in CMake.

You mean if the location of the binaries change from e.g. build/src/backend/cpu, correct? The CMake route makes sense, I'll try that.

Yes, instead of something like <path/libafcpu.so we just give <path> that may contain one or more binaries of our backends. Any and all available backends (afcpu, afcuda, afopencl) will be loaded.

Having said that, now I realize this would change the logic completely. Just throwing out an idea. In this approach, the user code will reduce from 3 lines to 1 line to add all three(cpu,cuda,opencl) new backends. If we add more backends, just saying that loading all in one line is better having a line for each backend we want to load.

Of course, in this new approach, we would have to provide a new function that prints/logs all available backends - much similar to how we list our devices in each backend - so that later the user can call af_set_backend_library on required index. We can have deterministic order in which backend handles are loaded so that cuda -> opencl -> cpu in a given path.

So I see two concerns here, the first being the original concern that @umar456 raised that I think we should solve first.

First, these kinds of variables are fragile:

#elif defined(__linux__)
const string library_suffix = ".so";
const string library_prefix_cpu = "/src/backend/cpu/libafcpu";
const string library_prefix_cuda = "/src/backend/cuda/libafcuda";
const string library_prefix_opencl = "/src/backend/opencl/libafopencl";

Because these locations might change in the future - I agree. I doubt that the library names would change though? But it's true that it would be better if even the library names are gotten from CMake or passed as command-line arguments to the executable. I would say the same for the file extensions.

The second concern is possibly providing the directory path that contains the libraries to be added. I agree that it will change the logic for af_set_backend_library indeed (about how to determine the index that should be used to activate the desired library), and I agree with @9prady9's thoughts in how that might possibly play out. However, I think that this concern can be considered instead in the future, and we should just consider this basic functionality for now.

Just a thought - the probability of library name change is about the same as those build directory change. File extensions on the other hand, I doubt dll or so or dylib are going to change in our lifetime 💯

Thanks, I agree. I think it's still better if these strings/paths were formed by CMake as you guys have suggested though. The way I handle this now is by checking the value of CMAKE_RUNTIME_OUTPUT_DIRECTORY first. If it's non-empty (as is the case in Windows builds), then I use that path for finding the libraries. Else (if empty), then that means it was not set in any CMake configuration file, and thus the output directory will default to the corresponding source directory hierarchy in the build tree (which in our case is src/backend/cpu and the like). I made those paths into variables and use them in the tests' CMakeLists. Here's the proposed change:

# <arrayfire_root>/CMakeLists.txt
set(CPU_BACKEND_SOURCE_DIR     src/backend/cpu)
set(CUDA_BACKEND_SOURCE_DIR    src/backend/cuda)
set(OPENCL_BACKEND_SOURCE_DIR  src/backend/opencl)
set(UNIFIED_BACKEND_SOURCE_DIR src/api/unified)

conditional_directory(AF_BUILD_CPU     ${CPU_BACKEND_SOURCE_DIR})
conditional_directory(AF_BUILD_CUDA    ${CUDA_BACKEND_SOURCE_DIR})
conditional_directory(AF_BUILD_OPENCL  ${OPENCL_BACKEND_SOURCE_DIR})
conditional_directory(AF_BUILD_UNIFIED ${UNIFIED_BACKEND_SOURCE_DIR})

# test/CMakeLists.txt
set(CPU_LIB_NAME "${CMAKE_SHARED_LIBRARY_PREFIX}afcpu${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(CUDA_LIB_NAME "${CMAKE_SHARED_LIBRARY_PREFIX}afcuda${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(OPENCL_LIB_NAME "${CMAKE_SHARED_LIBRARY_PREFIX}afopencl${CMAKE_SHARED_LIBRARY_SUFFIX}")

if(CMAKE_RUNTIME_OUTPUT_DIRECTORY)
  set(CPU_LIB_PATH "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${CPU_LIB_NAME}")
  set(CUDA_LIB_PATH "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${CUDA_LIB_NAME}")
  set(OPENCL_LIB_PATH "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${OPENCL_LIB_NAME}")
else()
  set(CPU_LIB_PATH "${ArrayFire_BINARY_DIR}/${CPU_BACKEND_SOURCE_DIR}/${CPU_LIB_NAME}")
  set(CUDA_LIB_PATH "${ArrayFire_BINARY_DIR}/${CUDA_BACKEND_SOURCE_DIR}/${CUDA_LIB_NAME}")
  set(OPENCL_LIB_PATH "${ArrayFire_BINARY_DIR}/${OPENCL_BACKEND_SOURCE_DIR}/${OPENCL_LIB_NAME}")
endif()

add_definitions("-DCPU_LIB_PATH=\"${CPU_LIB_PATH}\"")
add_definitions("-DCUDA_LIB_PATH=\"${CUDA_LIB_PATH}\"")
add_definitions("-DOPENCL_LIB_PATH=\"${OPENCL_LIB_PATH}\"")

"The function will return true if the unified backend is being used to call the ArrayFire functions. If the binary is linked directly to one of the backend libraries, this function will return false."

Ah, this is supposed to be the detailed description, right? I'll put this in.

Returns true if the unified backend is used to call ArrayFire functions.

This sounds more precise, thanks.

Adds a new backend library to from an arbitrary path on the file system.

This function registers a new backend library located anywhere on the system by providing a file path to the location of the library(so, dll, or dylib). Once registered you can call af_set_backend_library to activate the backend in your program.

The order in which you add a backend to this function will determine the index used to activate the backend using the af_set_backend_library call.

af_add_backend_library can be used to load different versions of the ArrayFire library.

Can you check what happens if you load the same backend multiple times. Is state between the two backends shared or is it treaded as a different instance. Might want to add that to the description.

I just added a test on this, and they do share the state (which makes sense since af_add_backend_library eventually calls loadLibrary, which returns the same handle when given the same path, and all af_set_backend_library does is change which handle is assigned to AFSymbolManager's activeBackend). Thanks, I'll add this to the documentation indeed.

EDIT: I meant the state of the arrays, at least. I'm curious to check the chosen devices for the same backend library too.

This needs a brief description at the top.

Thanks, I forgot about that since the function declarations around it didn't have brief descriptions as well.

You should add a better error message here. This is not descriptive enough.

Did you mean the comment? Or the way the error is reported? I'm actually kind of wary also of using UNIFIED_ERROR_LOAD_LIB again and again in every error scenario, if that's your concern.

Use this in tandem -> To be used in tandem with .. sounds better I think

Thanks, but hmm I'm not sure. Changing it to that actually makes it an incomplete sentence. We could change it instead to This is used in tandem... so that it sounds less commanding and more neutral, if that's your concern.

My concern is that the user should understand that both of those functions have to be used to change the active backend. Just calling add backend wouldn't suffice.

I don't think we added default behavior inside af_add_backend_library that switches current backend to newly added backend handle.

Yes, you're right, calling af_add_backend_library does not switch the current backend. af_set_backend_library does that. I did change the wording here though to something similar to what @umar456 suggested in this comment

The docs say max 7 binaries can be loaded. How this calculation work ?

3 are reserved for the default backend libraries that the AFSymbolManager loads (regardless of how many backends does the machine or the build actually supports).

what happens when loading of default backends fails ?

If the program doesn't add and set custom libraries, the arrayfire functions would fail ("Failed to load dynamic library") because the handles are null . This is the behavior even before this change. However, if the user does add and set custom libraries, then everything still works. I simulated this by making a small change in the AFSymbolManager constructor:

    // Decremeting loop. The last successful backend loaded will be the most
    // prefered one.
    for (int i = NUM_BACKENDS - 1; i >= 0; i--) {
        int backend          = order[i] >> 1;  // 2 4 1 -> 1 2 0
        // bkndHandles[backend] = openDynLibrary(order[i]); // <--
        if (bkndHandles[backend]) {
            activeHandle  = bkndHandles[backend];
            activeBackend = (af_backend)order[i];
            numBackends++;
            backendsAvailable += order[i];
        }
    }

This application code still works:

    string lib_path = "/home/mark/Documents/arrayfire-3.6.3/build/src/backend/cuda/libafcuda.so";
    af_add_backend_library(lib_path.c_str());
    af_set_backend_library(0);
    array a = randu(3, 3);
    array b = constant(5, 3, 3);
    array c = a + b;
    info();
    af_print(a);
    af_print(b);
    af_print(c);

Nah, you got me wrong there. I asked what happens if the defaults loading fails - the first three you are reserving for the current (lets refer to the ones that exist next to libaf as current) binaries. The custom handles offset (numBackendHandles) won't change I guess.

I assume, unless a set is called using custom handle index, our default backend-order-preference prevails, is that correct ?

Oh whoops, thanks for the clarification. Yes, the default backend order prevails. The code for setting that doesn't change.

I would suggest passing these as definitions via CMake, library suffix values are already available as variables in CMake.