[vsipl++] [patch] functional code for SPE-based vector multiply

[Stefan Seefeld]

Don McCoy wrote:
> This patch builds on the dispatch added by Stefan.  Specifically, it:
> 
>    * Encapsulates the initialization and shutdown code for setting up
>      the SPE
>    * Separates the sending of the data from the command to actually do
>      the multiplication
>    * Actually multiplies the vectors (not using intrinsics, but this is
>      a simple change)

Very nice !

Here is one high-level comment, as well as (as usual ;-) ) some nit-picking:

> +Elementwise_vmul::Elementwise_vmul()
> +{
> +  gid_ = spe_create_group(SCHED_OTHER, 0, 1);
> +  if (gid_ == NULL) 
> +  {
> +    std::cerr << "Failed spe_create_group(errno=" << errno << ")" << std::endl;
> +    return;
> +  }

We shouldn't write to std::cerr here. Instead, we should define some exception
type that would be thrown in case of error.

[...]


> +
> +static Elementwise_vmul elementwise_vmul;

The Elementwise_vmul object should be allocated from inside the global vsipl object.
This also gives us the opportunity to handle Cell-specific command-line options, such
as define the concurrency level, or different dispatch strategies.

However, doing that requires the type to become truely generic, such that...

> +template <typename T>
> +void Elementwise_vmul::apply(T const *A, T const *B, T *R, length_type len)

...this becomes a generic call, taking some 'op-code' parameter that indicates
what operation to perform.

> +  volatile command_block cb __attribute__ ((aligned (128)));
> +  volatile data_block db[3] __attribute__ ((aligned (128)));
> +
> +  speid_t speid = elementwise_vmul.speid_;
> +
> +  // pass data blocks to SPE
> +  {
> +    db[0].element_size = sizeof(float);

Does this mean you assume the template parameter T is now float ?

Finally, this...

> +class Elementwise_vmul
> +{
> +public:
> +  Elementwise_vmul();
> +  ~Elementwise_vmul();
> +
> +  template <typename T>
> +  static void apply(T const *A, T const *B, T *R, length_type len);
> +
> +private:
> +  spe_gid_t gid_;
> +  speid_t speid_;
> +  int status_;
> +};

...can become as generic as:

class SPE
{
public:
  enum Op_code {..., vmul, vadd, fft_fwd, rfft_fwd, ...};

  SPE();
  ~SPE();
  template <typename R, typename A, typename B, ...>
  void call(Op_code, R, A, B, ...);
};


And this...

> +  Elementwise_vmul::apply(A, B, R, len);

...then becomes

SPE *spe = get_target_spe(SPE::vmul);
spe->call(SPE::vmul, r, a, b);


With that all the work is encapsulated into the 'get_target_spe()'
function (where we can determine which SPE is most fit for the call,
based on what is currently loaded there), as well as
SPE::call() (where we can determine what needs to be done to prepare
the SPE in order to execute this operation).

Thanks,
		Stefan

-- 
Stefan Seefeld
CodeSourcery
stefan at codesourcery.com
(650) 331-3385 x718