llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[SYCL][NFCI] Refactor `weak_obj` implementation

Open aelovikov-intel opened this issue 1 month ago • 2 comments

Most important part is the removal of a dedicated weak_object_base.hpp. It was introduced to break a cyclic headers dependency, but a forward declaration achieves the same in a much less intrusive way.

Once the header is removed and weak_object_base definitions are moved into weak_object.hpp where sycl::buffer/sycl::stream are complete we can move more common methods into the weak_object_base (see lock()).

The rest is minor stylistic improvements.

aelovikov-intel avatar Dec 05 '25 18:12 aelovikov-intel

Going through individual commits might be easier to review, I kept them atomic.

aelovikov-intel avatar Dec 05 '25 18:12 aelovikov-intel

Windows failure is basically this (I think):

#include <memory>
#include <type_traits>

struct ImplUtils {
  template <class Obj>
  static const decltype(Obj::impl) &getSyclObjImpl(const Obj &SyclObj) {
    return SyclObj.impl;
  }
};

template <class Obj>
auto getSyclObjImpl(const Obj &SyclObj)
    -> decltype(ImplUtils::getSyclObjImpl(SyclObj)) {
  return ImplUtils::getSyclObjImpl(SyclObj);
}
template <typename SYCLObjT> class weak_object_base;

template <typename SYCLObjT>
decltype(weak_object_base<SYCLObjT>::MObjWeakPtr)
getSyclWeakObjImpl(const weak_object_base<SYCLObjT> &WeakObj);

template <class SyclObjT> class OwnerLessBase {
public:
  bool ext_oneapi_owner_before(
      const weak_object_base<SyclObjT> &Other) const noexcept {
    return getSyclObjImpl(*static_cast<const SyclObjT *>(this))
        .owner_before(getSyclWeakObjImpl(Other));
  }

  bool ext_oneapi_owner_before(const SyclObjT &Other) const noexcept {
    return getSyclObjImpl(*static_cast<const SyclObjT *>(this))
        .owner_before(getSyclObjImpl(Other));
  }
};

class device_impl;

class __declspec(dllexport) device : public OwnerLessBase<device> {
  friend ImplUtils;

public:
private:
  std::shared_ptr<device_impl> impl;
};
$ clang++ -fsyntax-only -xc++ a.ii -fms-extensions -Xclang -triple -Xclang  x86_64-windows-msvc
a.ii:27:23: error: no matching function for call to 'getSyclWeakObjImpl'
   27 |         .owner_before(getSyclWeakObjImpl(Other));
      |                       ^~~~~~~~~~~~~~~~~~
a.ii:22:33: note: in instantiation of member function 'OwnerLessBase<device>::ext_oneapi_owner_before' requested here
   22 | template <class SyclObjT> class OwnerLessBase {
      |                                 ^
a.ii:20:1: note: candidate template ignored: substitution failure [with SYCLObjT = device]: implicit instantiation of undefined template 'weak_object_base<device>'
   19 | decltype(weak_object_base<SYCLObjT>::MObjWeakPtr)
      |          ~~~~~~~~~~~~~~~~
   20 | getSyclWeakObjImpl(const weak_object_base<SYCLObjT> &WeakObj);
      | ^
1 error generated.

which should be solvable by ensuring weak_object.hpp is include in every sycl.dll's TU that uses OwnerLessBase (which isn't unreasonable). Not sure how to do that yet.

aelovikov-intel avatar Dec 05 '25 21:12 aelovikov-intel