[SYCL][USM] Enable per-context USM behavior.

[jbrodman]

Keep track of USM "state" per-context to handle multiple device scenarios.

Signed-off-by: James Brodman [email protected]

[romanovvlad]

Please, follow llvm coding guidelines. name variables starting from big letter, functions from small.

[romanovvlad]

Suggest rewriting it to:

cl::sycl::detail::usm::CLUSM *&retVal = gCLUSM[ctxt];
if(nullptr == retVal)
retVal = new cl::sycl::detail::usm::CLUSM();

And move check for backend to the function beginning. Now you are allocating CLUSM even for non OpenCL BE.

[romanovvlad]

Can't you store pointer to CLUSM in the context object?

[jbrodman]

Yep. Wasn't sure if you guys preferred it to be that invasive.

[jbrodman]

Actually - not sure I can really do this.

[jbrodman]

See below in the opencl_shim part.

[romanovvlad]

Could you please make this variable name more descriptive?

[jbrodman]

which one?

[romanovvlad]

t

[romanovvlad]

It seems you do the same 3 calls to setKernelExecInfo in both branches of if. Could you please move it outside of the if?

[jbrodman]

They are different calls. The ones through the clusm object do extra book keeping.

[romanovvlad]

Please avoid using global objects. It seems you can easily store them in context object or in some object which depends on context object.

[jbrodman]

Sure.

[jbrodman]

On second thought. By the time we're here, we've left sycl land. We don't have a handle on the sycl context. Not sure I can avoid global objects here unless we break the CL extension interface in a really non-CL friendly way.

[romanovvlad]

Let's take hostMemAlloc for example, we can have something like this:

// The class holds all data you currently using globals for.
// CLUSM and USMHandle can be merged.
class USMHandle  {
  FuncType *pfn_clDeviceMemAllocINTEL = nullptr;
  ...
  CLUSM CLUSM;
};
class context_impl {

  context_impl () {
    ...
    MUSMHandle.reset(getUSMHandleFor(m_Context));
  }
  void *alignedAlloc(...) {
    // The following logic can be implemented in USMHandle
    if(m_USMHandle->pfn_clDeviceMemAllocINTEL) {
      pfn_clHostMemAllocINTEL(m_Context, ...);
    }
    else {
      USMHandle->CLUSM->hostMemAlloc(...);
    }
  }
  ...
  unique_ptr<USMHandle> m_USMHandle;
};

[jbrodman]

Yeah, this is what I meant. Now we're driving all this branching logic into the implementation instead of having a nice clean shim. Would you guys prefer this? It's a non-trivial rewrite.

[jbrodman]

Probably the only way forward here is to also do the PI rewrite - this would let us do a clean interface using SYCL objects instead of CL objects and let us do this type of book keeping.

[romanovvlad]

I would prefer this as having global objects cause a lot of problems.