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

[Tom Vercauteren]

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.

> 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



> 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