[Insight-developers] Rounding and itkMath.h (Class vs Namespace)

[Bradley Lowekamp]

On Oct 22, 2009, at 1:13 PM, Tom Vercauteren wrote:

> Hi Brad,
>
> On Thu, Oct 22, 2009 at 16:48, Bradley Lowekamp <blowekamp at mail.nih.gov 
> > wrote:
>> Hi Tom,
>> I have a couple questions/comments too:
>> 1) I had to change these statements to the following:
>> inline vxl_int_32 RoundHalfIntegerToEven_32(float  x) { return
>> _mm_cvtss_si32(_mm_set_ss(x)); }
>> inline vxl_int_64 RoundHalfIntegerToEven_64(float  x) { return
>> _mm_cvtss_si64(_mm_set_ss(x)); }
>> There was a compiler error in regards to mixed up float and double  
>> SSE
>> vector types, I took a guess that this was the methods intended.
>
> Sorry I am not sure I got you point here. Did you mean that you had to
> replace (from  itk-templatedmathround-2009-07-30.patch)
>
> inline vxl_int_64 RoundHalfIntegerToEven_64(float  x) { return
> _mm_cvtsd_si64(_mm_set_ss(x)); }
>
> by what's in the repository
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkMathDetail.h?revision=1.2&root=Insight&sortby=date&view=markup
>
> inline vxl_int_64 RoundHalfIntegerToEven_64(float  x) { return
> _mm_cvtss_si64(_mm_set_ss(x)); }
>
> ?
> Then yes, this seems right. It must have slipped through because I
> mainly use a 32 bit box.

Yes this was the change I was referring to.

There also appear to be some problem with it:

http://www.cdash.org/CDash/viewBuildError.php?buildid=454563

http://gcc.gnu.org/ml/gcc-patches/2006-01/msg01759.html

This build appears to use an older version of gcc, 3.4.6 which does  
not appear to have these intrinsics defined. I suppose a compile time  
test is in order here.

Detecting this SSE stuff is very complicated and error prone. We still  
have the outstanding issue of the apple continuous build performing a  
complete rebuild each time due to some issue related to SSE detection.  
This part is just so tricky due to all the different compilers and  
systems supported.



>
>> 2) And you double check that the following is correct:
>> #if VNL_CONFIG_ENABLE_SSE2_ROUNDING && defined(__SSE2__) &&
>> (defined(__x86_64) || defined(__x86_64__) || defined(_M_X64)) &&
>> (!defined(__GCCXML__))
>> # define USE_SSE2_64IMPL 1
>> #else
>> # define USE_SSE2_64IMPL 0
>> #endif
>> I am unsure on why using SSE2 would depend building 64-bits. ?
>
> Now that you ask, I am starting to doubt. I haven't access to my
> computer right now but I kind of remember that using _mm_cvtss_si64
> and _mm_cvtsd_si64 didn't compile on my 32 bit box. This seems
> supported by the following link
> http://msdn.microsoft.com/en-us/library/bb514009.aspx
> that states that  _mm_cvtsd_si64 is for x64

This may be different on linux and apple system then on widows.  
Complicated!

>
>
>
>> 1) The question. For revision 1.3, the commit message is
>> "COMP: work around for vs6 not correctly supporting function  
>> overloading"
>> and your patch was to replace
>> #ifndef ITK_LEGACY_REMOVE
>> by
>> #if !defined(ITK_LEGACY_REMOVE) && !(defined(_MSC_VER) && (_MSC_VER  
>> <=
>> 1300))
>>
>> I am not sure I really understand you here. Is this to force vs6 into
>> using the new template methods to try and see if we actually need a
>> dummy argument (cf.
>> http://www.itk.org/mailman/private/insight-developers/2009-August/013231.html
>> ) for this compiler? By the way is seems to be the case:
>> http://www.cdash.org/CDash/viewBuildError.php?buildid=454305
>>
>> I am interpreting this error message that vsc6 can not have the  
>> following
>> type of function overloading:
>> int foo( int i);
>> template< typename T > int foo( T i );
>> Hence I don't think the new and the legacy functions can co-exists  
>> on vs6.
>> The preprocessor directive is an attempt to remove the legacy  
>> methods for
>> just vs6. As I don't have access to vs6 and there are only a couple  
>> of vs6
>> builds each day, it has been a rather slow experiment tying to  
>> figure this
>> one out.
>> The dummy argument doesn't really seem needed because the templeted  
>> methods
>> is compiling fine in the second half of the itkMathTest.
>
> Indeed, I read the dashboard errors too quickly!
>
>
>> 2) The comment. In the beginning of itkMath.h, you wrote the  
>> following
>> comment:
>> // note: including Macro.h here will need some thought, which comes  
>> first?
>>
>> To me, it would make sense to include itkMacro.h in itkMath.h because
>> itkMath.h may need some macros. Concept checking is one such example.
>> However this will break strict backward compatibility because, one
>> will have to include itkMath.h in place were rounding fucntions from
>> itkMacro.h were already used. I don't think it's such a big deal
>> though.
>>
>> One alternative to avoid this would be to let the non-template
>> rounding functions in itkMacro.h and surround then with
>> #ifndef ITK_LEGACY_REMOVE
>> #endif
>> Same thing for the
>> #include "itkMath.h"
>> in itkMacro.h
>>
>> Note that I wouldn't vote for this option. Breaking strict backward
>> compatibility in the sense that we only require people to add an
>> include seems fine to me especially since we already move from
>> non-template round to template round.
>>
>> I think the easiest thing to do will be to include the following at  
>> the END
>> of itkMacro.h like the following:
>> #ifndef ITK_LEGACY_REMOVE
>> #include "itkMath.h"
>> #endif
>> This should allow itk Math to use Macro.h, and there be legacy  
>> support too.
>> I don't see a downside here. Just a little ugly place for the  
>> include.
>
> Wow! Putting an include at the end. Never thought of doing that. Ugly,
> but quite useful, work-around I guess.
>
> Tom

========================================================
Bradley Lowekamp
Lockheed Martin Contractor for
Office of High Performance Computing and Communications
National Library of Medicine
blowekamp at mail.nih.gov


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20091022/b51c0047/attachment.htm>