llvm

llvm copied to clipboard

Not all feedback on #635 was implemented for the current version of the extension, so this issue is to track the outstanding suggestions:

1. Additional class interface, such as:

   public:
     using value_type = dataT;
     std::size_t constexpr min_capacity = MinCapacity;
   

   and probably with other container-like types: reference...

   • https://github.com/intel/llvm/pull/635#discussion_r324952087

2. "have instead of std::size_t a numeric type as a template parameter" for the pipe class

   • https://github.com/intel/llvm/pull/635#discussion_r324953479

3. More C++-like interface for host pipe map/unmap/read/write

   • https://github.com/intel/llvm/pull/635#discussion_r324955956
   • https://github.com/intel/llvm/pull/635#discussion_r324956992

I find the spec quite sound.

But wouldn't it make more sense to have the following pipes:

using pipe<class bar, float>;
using pipe<class bar, float, 3>;
using pipe<class bar, float, 5>;

all refer to the same physical pipe and impose said pipe to be of minimum capacity 5 (the max of all the capacities)?

Specifying a minimum capacity is a clever way of having a clear unambiguous unification strategy (this would not have been possible if they were an exact capacity, for instance). Why not make use of it? This feel more natural to me, but maybe there is an implementation difficulty?

We could have all the pipe<Name,Type,MinCap> inherit from a common unsized_pipe<Name,Type> which would carry the static member functions (so that they are shared among all the "sized" pipes). The implementation could then decide which capacity to use by picking one that is at least as big as all the min capacities requested on that pipe?

Since we need this internally to replace our obsolete SYCL 2.2 implementation, I started to implement this on CPU first. Look at https://github.com/triSYCL/triSYCL/pull/263

Globally, I like the API and think that this compile-time static approach is good for devices such as FPGA.

But the interesting part is that the non blocking API is super verbose as shown in example https://github.com/triSYCL/triSYCL/pull/263/files#diff-357fa002c7f62d78f745923e61db84d5 and awkward to use as I commented already in https://github.com/triSYCL/triSYCL/pull/263 long time ago. Even in the implementation it smells the forced redundant copies implied by the API.

@keryell, can you expand on your concerns a bit? Do you prefer "try" in the name as was discussed in #5839 simply because it makes it clear that it's non-blocking or for another reason? Are you also advocating that the success code not be passed as an argument? E.g.:

// Header
template<typename name, typename dataT, typename propertiesT = decltype(properties{})>
struct pipe {
  struct result {
    bool success;
    dataT data;
  };
  static result tryRead();
}

// User code
using my_pipe = ...
my_pipe::result r = my_pipe::try_read();
if (r.success) {
  ... = r.data;
}

I'm not sure that's less verbose. Alternatively, are you advocating for the success_code and the data to be swapped?

The problem with just read() or write() overloads is that the code is unreadable if you are not a SYCL lawyer. So I think that try_read() and try_write() (or other similar name) is more explicit and gives the intent more easily. Another problem with the API is to make it efficient too. What if your pipe packet is 10 GiB big? Every time you try to read something, you copy 10 GiB of garbage data when there is nothing to read. :-( In that case

bool my_pipe::try_read(T&);
bool my_pipe::try_write(const T&);

might be better. Otherwise for small cases you could have also an overload returning an optional:

std::optional<T> my_pipe::try_read();

similar to what you propose.

At this point code suggestions were applied, changing API names are up to specification folks.

@GarveyJoe - What is the current status of this? Have all the comments been addressed in the current implementation?