[vsipl++] [patch] Support for Cell FFT's up to 4K points.

[Stefan Seefeld]

Don McCoy wrote:
> Stefan Seefeld wrote:
>>> -  Fft_impl(Domain<1> const &dom)
>>> +  Fft_impl(Domain<1> const &dom, rtype scale)
>>> VSIP_THROW((std::bad_alloc))
>>>     
>>
>> Since any throw specifier other than 'throw ()' will lead to worse
>> code, I
>> think we should not use it if we can, in particular in non-public-API
>> code.
>>
>>   
> The reason for the above is the the alloc below.  I guess I'm not sure
> what the right thing to do here is.

I understand, this constructor can throw. My point is that expressing that
in a throw specifier unfortunately isn't really helpful.

>> Don't enclose any function doing actual work into assert(), as that
>> will be removed
>> when compiling with -DNDEBUG. Also, let's be careful (and explicit)
>> with possible
>> return values: some values may be impossible with correct code (and
>> thus should lead
>> to an abort(), while others may not, and thus should result in an
>> exception. So, it
>> would be best to explicitely list (named) return values, and possibly
>> even add a comment
>> that explains what values we check for and what not. (Who knows, may
>> be ALF's own API
>> evolves, so we have to carefully make adjustments...)
>>
>>   
> Yes, that's true.  I was trying to get more informative messages out of
> assert(), but that can better be done by naming the return value
> something informative.  Is that what you meant?

Yes. Even if the variable name isn't printed to the user, a developer
looking at the code will see immediately what values we check for,
and can compare against the ALF API docs to verify that all possible
return values are correctly accounted for.

>>  
>>> Index: src/vsip/opt/cbe/spu/alf_fft_c.c
>>> ...
>>> +#include "../common.h"
>>>     
>>
>> I'd suggest we avoid such relative paths in include directives.
>> <vsip/opt/cbe/common.h>
>> would be safer, I believe. Else we have to be extra careful in our
>> next attempt to
>> move things around.
>>
>>   
> I knew you'd catch that!
> 
> I think we need something in the makefile to specify the path, because
> <vsip/opt/cbe/common.h> does not work IIRC.  I'll fix it.

It may currently not work because the spu/GNUmakefile.inc.in is missing
an include path like '-I $(srcdir)/src/vsip'. That should be added, then.


>>> +  // Perform the FFT, +  //   -- 'in' may be the same as 'out'
>>> +  if (fftp->direction == fwd_fft)
>>> +    fft_1d_r2(out, in, W, log2_size);
>>> +  else
>>> +    fft_1d_r2_inv(out, in,  W, log2_size, fftp->scale);
>>>     
>>
>> Out of curiosity: do these two functions really share all the
>> essential code ? I'm wondering whether putting them into two
>> separate kernels would help us cut down the code / stack size.
>>
>>
>>   
> fft_1d_r2_inv() does the scaling and reordering needed after calling
> fft_1d_r2() (that is called directly for the forward cases).  It is very
> little additional code, and necessary to have for fast convolution in
> order to avoid reloading the kernel.

Indeed, but fast convolution will be an entirely new kernel anyway, and
we may be able to have an FFT kernel that can handle larger data blocks
than the convolution kernel, due to its size.

Thanks,
		Stefan

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