Streams for BLAS. maskedCopy. generic reduce kernels. bugfix.

[soumith]

Stream support for BLAS Handles.
maskedCopy implemented
generic Reduce kernels
Fixed a small ffi inconsistency introduced with #158

[soumith]

all tests pass.

[soumith]

cc: @dominikgrewe more stuff on it's way. do review when you get a chance.

We've been using this branch for most of the last month or more, so it's definitely stable and afaik fairly bug-free.

[dominikgrewe]

Could we couple the BLAS handles with streams? I.e. for each stream, we'll have a separate BLAS handle and when switch streams you automatically switch BLAS handles.
What do you get from managing them separately?

[dominikgrewe]

Btw, it would be great if you could break these patches down a bit more. There is so much going on here, it's hard to make sense of it all. Won't get a chance until early next week to take a closer look.

Was the THCDeviceTensor code meant to be in this PR? You don't mention it in your comments.

[soumith]

Regarding BLAS Handles, response from Nicolas(FB):
"They are fundamentally different resources with different lifetimes.
Atm, you can run multiple cublas calls with multiple streams on a single handle or only use a single stream across multiple handles.
I don’t have a strong argument against merging them but there may be performance / memory consumption implications of always forcing a single stream to a single handle.
The problem is I can’t think of a good test that would prove or disprove this.

Running mxm (513x513x513): 53 iterations (parallel over streams), 1 batches, GReductions(virtual fmas)/s = 634.47793 time = 11.28ms
Running mxm (513x513x513): 53 iterations (parallel over streams), 1 batches, GReductions(virtual fmas)/s = 785.07805 time = 9.11ms
Running mxm (513x513x513): 53 iterations (parallel over streams), 1 batches, GReductions(virtual fmas)/s = 931.75268 time = 7.68ms
Running mxm (513x513x513): 53 iterations (parallel over streams), 1 batches, GReductions(virtual fmas)/s = 934.15645 time = 7.66ms

These are simple perf results for respectively 1x1, 1x4, 4x1 and 4x4 handles x streams
If you have only 1 handle it’s better to have 4 streams.
But then it’s even better to have 4 handles and there is no significant gain from 4x4 over 4x1."

[soumith]

I'm going to break these further in the future. It's just that we've gone too far from HEAD, and I wanted to just put the PR out there.

[soumith]

The THCDeviceTensor* is an isolated subset of facebook::cuda( https://github.com/facebook/fbcuda ). We isolated the subset needed for the reduction kernels, and the Volumetric Max/Avgpooling kernels, and named them THCDevice* so that we dont have a dependency on fbcuda on the core of cutorch.
This is approximately what we discussed with @akfidjeland and @koraykv back in that email thread a couple of months ago.

[dominikgrewe]

This doesn't match the definition in THCGeneral.h

[soumith]

cc: @nicolasvasilache

[wickedfoo]

the scratch space needs to be added here

[soumith]

cc: @nicolasvasilache please take a look at @dominikgrewe 's comments.

[dominikgrewe]

You only really want to call this method if device is the current device, right? Otherwise currentBlasHandle holds a handle for a device other than the one we're currently using.

[nicolasvasilache]

Correct, this is how it is used atm but it should be enforced (by having only a THCState_setBlasHandleForCurrentDevice function).

[wickedfoo]

@soumith I'll send out a diff locally fixing the FFI and converting logicalany/logicalall that you can pull into this.

[dominikgrewe]

As far as I can tell, THCDeviceTensor is not used in this PR (the reduce kernels don't use it). Can you factor it out into a separate PR? It should be fairly straightforward and we'd keep the commit history a bit saner.

[soumith]

sure i'll do that,

[hughperkins]

Hi, random comment, are you sure you need the init value? I guess you can do things like? :

  float r = modifyOp(in.data[inOffset]);
  inOffset += reductionStride;
  for (IndexType i = 1; i < reductionSize; ++i) {
    r = reduceOp(r, modifyOp(in.data[inOffset]));
    inOffset += reductionStride;
  }

[wickedfoo]

@hughperkins To take the two parts in turn:

1. Needing init. I think I can remove this. The loops are block strided, so the block would always have to be # of threads >= totalElements in order for that to work. This I could do, but would have to check that in a couple of places, and there's this possible fragile dependency between the minimum number of elements and the block size. I'll look into that.
2. Simplifying the loop. There's this piece of code that occurs in various places:

    const IndexType inOffset = IndexToOffset<IndexType, ADims>::get(i, in);

the loop iterates in linear index order (i.e., [0, numElements -1]), which is converted to a real byte offset in a multi-dimensional array (with arbitrary strides/holes) using the appropriate math. You could be iterating over 5-d slices of a 6-d array, reducing over the 3rd dimension. It can't just be += reductionStride since the slice being reduced doesn't necessarily have a constant stride. This should compile down to effectively that in the case that the slice being reduced does have a constant stride, though.

[dominikgrewe]

Can we not use THCudaTensor_pointwiseApply3 here and pass in maskPrefixSum as an argument? Then we wouldn't need to calculate the offset.

[wickedfoo]

yes

[dominikgrewe]

@soumith I just added a few more comments. Once those ones and the previous ones have been addressed please let me have another quick look, but it should be good to go then. Thanks.

[hughperkins]

I get the following compile warning, using yesterday's goodies2:

/home/user/git/cltorch/lib/THCl/THClReduceAll.h:38:19: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if (numBlocks > scratchSpace) {

Looking at the original code:

inline long getTwoPassBlocks(THCState* state, long elements) {
  long numBlocks = THCCeilDiv(elements, THC_REDUCE_ALL_BLOCK_SIZE);

  // We can only have as many blocks as there is scratch space
  size_t scratchSpace =
    THCState_getCurrentDeviceScratchSpaceSize(state) / sizeof(float);
  THAssert(scratchSpace > 0);

  if (numBlocks > scratchSpace) {
    numBlocks = scratchSpace;
  }

  return numBlocks;
}

... looks like numBlocks is long, ie signed, but scratchSpace is size_t, ie unsigned?

[hughperkins]

As far as I can tell, THCDeviceTensor is not used in this PR (the reduce kernels don't use it). Can you factor it out into a separate PR? It should be fairly straightforward and we'd keep the commit history a bit saner.

eg CeilDiv is needed by THC[l]ReduceAll.cl/cuh currently, and CeilDiv is currently part of THC[l]DeviceTensor

[hughperkins]

Personal observation: it would be easier to find functions if they are prefixed with the module name :-) eg, in THCReduceAll, there is a method THCudaTensor_reduceAll , which I would logically hunt for in THCTensor. (At least, I spend a lot of time doing grep to find functions, since their name gives no indication of where to find them?)

[dominikgrewe]

As far as I can tell, THCDeviceTensor is not used in this PR (the reduce kernels don't use it). Can you factor it out into a separate PR? It should be fairly straightforward and we'd keep the commit history a bit saner.

eg CeilDiv is needed by THC[l]ReduceAll.cl/cuh currently, and CeilDiv is currently part of THC[l]DeviceTensor

Ah, I didn't see that it was part of the THCDeviceTensor code. Good catch, thanks!
It's unrelated to the rest of the THCDeviceTensor code though, so we should probably put it into a different file anyway.

[dominikgrewe]

Personal observation: it would be easier to find functions if they are prefixed with the module name :-) eg, in THCReduceAll, there is a method THCudaTensor_reduceAll , which I would logically hunt for in THCTensor. (At least, I spend a lot of time doing grep to find functions, since their name gives no indication of where to find them?)

I think all functions operating on tensors (and that's the vast majority) start with THCudaTensor, apart from "special" ones like methods wrapping cuBLAS. So maybe files like THCReduceAll should be renamed to THCTensorReduceAll? That would be more consistent with THCTensorMath etc and should hopefully make it fairly obvious where to look for functions like THCudaTensor_reduceAll.

Prefixing method names according to the files they're defined in means that users of THC need to know which function is defined where, which doesn't seem very sensible.

[hughperkins]

Question: why do we pass get_local_size(0) into reduceBlock in THClTensor_reduceContigDim, rather than 'reductionSize'? It seems that this works ok since all threads have r set to init, but if we changed it to reductoinSize, then we might not need the init var? (I'm still a newbie to the code, so I'm posing a question really, rather than making a suggestion.)

[wickedfoo]

I think all functions operating on tensors (and that's the vast majority) start with THCudaTensor, apart from "special" ones like methods wrapping cuBLAS. So maybe files like THCReduceAll should be renamed to THCTensorReduceAll? That would be more consistent with THCTensorMath etc and should hopefully make it fairly obvious where to look for functions like THCudaTensor_reduceAll.

In my opinion, the distinction between reduceAll etc. is that they are not part of the C Torch tensor API (e.g., the same functions implemented in torch7 for CPU tensors). They are implementation utilities for implementing the C Torch CudaTensor API, and aren't meant for end-users of the C tensor api.

I can change if you want, or leave it up to @soumith.

[wickedfoo]

They are C++ templates, not C API calls as well.

[soumith]

@dominikgrewe please see #181 , @colesbury is taking this over to completion...