[Insight-developers] Rounding functions in itkMacro.h

[Luis Ibanez]

Tom,

Should the CMake flag:

    VNL_CONFIG_ENABLE_SSE2_ROUNDING

be set to ON   ?

When ON,  I'm getting compilation errors for

                          _mm_set_ss

not being defined.

I'm now trying building with this flag OFF.


   Thanks for any hint,


          Luis


-------------------------------------------
On Mon, May 11, 2009 at 6:24 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
>
> Hi Tom,
>
>
> Thanks for the detailed instructions
>
> following your guidance,
> I have patched the following files:
>
> * vxl/core/vnl/CMakeLists.txt
> * vxl/core/vnl/vnl_config.h.in
> * vxl/core/vnl/vnl_math.h
> * vxl/core/vnl/tests/test_math.cxx
>
>
> I'm running an Experimental build,
> and if things go well, I'll be committing
> these changes after the repository closes
> at 9pm. In this way we will see the effects
> on the first continuous builds tomorrow.
>
> At that point, we could take the new
> functions out of the itkMacro.h file
> and move them into a file
>
>            itkMath.h
>
> To be put in Code/Common.
>
>
>
>    Thanks
>
>
>        Luis
>
>
>
> -------------------
> Tom Vercauteren wrote:
>>
>> Hi Luis,
>>
>> I didn' t understand that ITK_USE_PORTABLE_ROUND was meant to be
>> temporary. It makes more sense now!
>>
>> Below are some comments.
>>
>>
>>>> First of all, it just happens that my patch for vnl has been committed
>>>> by Amitha Perera 3 days ago :)
>>>>
>>>>
>>>> http://vxl.svn.sourceforge.net/viewvc/vxl?view=rev&sortby=date&revision=25053
>>>> It therefore seems like we could just update the corresponding pieces
>>>> of vnl now and wait for the next vxl release to update the rest of it.
>>>>
>>>
>>>
>>> This sounds good.
>>> I'll take a look at what files we need to move in.
>>> (hopefully we will not have a domino effect...)
>>
>>
>> The difference should be rather minimal.:
>> - Utilities/vxl/core/vnl/vnl_math.h can be replaced by its vxl svn
>> counterpart
>> - Utilities/vxl/core/vnl/tests/test_math.cxx can be replaced by its
>> vxl svn counterpart
>> - Utilities/vxl/core/vnl/vnl_config.h.in can be replaced by its vxl
>> svn counterpart
>> - Utilities/vxl/core/vnl/CMakeLists.txt needs to be modified. We needs
>> to be taken from its vxl  svn counterpart is everything that touches
>> VNL_CONFIG_ENABLE_SSE2_ROUNDING
>>
>>
>>
>>>> 1) The code uses the following preprocessor defines:
>>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING
>>>> GCC_USE_FAST_IMPL
>>>> VC_USE_FAST_IMPL
>>>> VNL_CHECK_FPU_ROUNDING_MODE
>>>> These are not even provided by the vnl version that ships with ITK. We
>>>> should either use a newer version of vnl or provide some equivalent of
>>>> these within ITK proper.
>>>>
>>>
>>>
>>> Would this be an issue when we bring up the fix from VXL ?
>>> Isn't it the case that the newer VXL files will have these symbols ?
>>
>>
>> This won't be an issue once Utilities/vxl/core/vnl/vnl_config.h.in and
>> Utilities/vxl/core/vnl/CMakeLists.txt are patched.
>>
>>
>>
>>>> 2) The code in itkMacro.h that provides the new rounding methods is
>>>> put within a  ifdef ITK_USE_PORTABLE_ROUND block. I don't think we
>>>> need or want this. It seems that all code should be able to use these
>>>> new rounding functions. Actually what is the rationale for having this
>>>> ITK_USE_PORTABLE_ROUND define? Can't we just replace all calls to
>>>> vnl_math_rnd by itk::Math::RoundHalfIntegerUp or
>>>> itk::Math::RoundHalfIntegerBackwardCompatibility to maintain strict
>>>> backward compatibility?
>>>>
>>>
>>>
>>> We put the CMake symbol as a way of being able to fix the proper places
>>> withtout affecting other developers in the meantime. The variable is not
>>> intended to stay there.
>>>
>>> You may have noticed the 6 failing tests in Zion and Redwall in the
>>> Dashboard. These machines have the ITK_USE_PORTABLE_ROUND ON, and these
>>> are the remaining failing tests. Once we fix them, we can remove the
>>> variable and adopt the new rounding.
>>
>>
>> Ok, I get it now. This makes sense.
>>
>>
>>
>>>> 4) itkMacro.h only provides round but not floor and ceil. I think we
>>>> should have them (itk::Math::Ceil and itk::Math::Floor).
>>>>
>>>
>>>
>>> I fully agree.
>>> We were going one step at a time here...
>>
>>
>> Ok, got it.
>>
>>
>>
>>>> 5) The itk::Math::XXX functions are not macros. Wouldn't it be better
>>>> to put them in there own file (e.g. itkMath.h)? This header might in
>>>> turn be included by itkMacro.h for convenience reasons.
>>>>
>>>
>>>
>>> Yes, the final destination should be a file itkMath.h.
>>> We were avoiding to commit a new file until we were sure
>>> that the new functions would do the trick.
>>
>>
>> Again it makes sense now that I understand it is a progressive approach :)
>>
>>
>>
>>>> Even though
>>>>
>>>> http://vxl.svn.sourceforge.net/viewvc/vxl?view=rev&sortby=date&revision=25053
>>>> now provides good floating point to integer function, I understand
>>>> your worry about not necessarily relying on vnl if we don't have to.
>>>> What I suggest to fulfill most requirements is the following:
>>>>
>>>> A) Update
>>>> Utilities/vxl/core/vnl/vnl_math.h
>>>> Utilities/vxl/core/vnl/tests/test_math.cxx
>>>> Utilities/vxl/core/vnl/vnl_config.h.in
>>>> Utilities/vxl/core/vnl/CMakeLists.txt
>>>> to get the most up-to-date floating point to integer functions from vnl
>>>
>>>
>>> Thanks for the list.
>>> This is indeed very useful.
>>> I'll run an Experimental build with these updated files.
>>>
>>>
>>>> B) Modify itkMacro.h so that
>>>> itk::Math::Round be only a wrapper on vnl_math_rnd (version as in
>>>> current vxl svn)
>>>> itk::Math::RoundHalfIntegerUp be only a wrapper on
>>>> vnl_math_rnd_halfintup
>>>> itk::Math::RoundHalfIntegerToEven be only a wrapper on
>>>> vnl_math_rnd_halfinttoeven
>>>>
>>>
>>>
>>>
>>> I think we still should do this.
>>> It will give us a layer of independence from VXL.
>>> and will allow us to make modifications without having to wait
>>> for updates in VXL.
>>
>>
>> This indeed seems a reasonable approach.
>>
>>
>>
>>>> C) Expand itkMacro.h so that
>>>> itk::Math::Ceil be only a wrapper on vnl_math_ceil
>>>> itk::Math::Floor be only a wrapper on vnl_math_floor
>>>>
>>>
>>>
>>> This is still desirable.
>>> Again, probably to be put in a itkMath.h file.
>>
>>
>> Ok, thanks,
>>
>>
>>
>>>> D) If we need to be able to use the exact (platform-inconsistent) same
>>>> behavior of vnl_math_rnd as currently in ITK, put
>>>> itk::Math::Round
>>>> within a ifdef block and put the current ITK's vnl_math_rnd code in
>>>> there. Another option might be to had a new function
>>>> itk::Math::RoundHalfIntegerBackwardCompatibility
>>>>
>>>
>>>
>>> I would think that consistency across platforms
>>> is a requirement, and that the previous inconsistency
>>> is a bug, and not a feature that we want to be backwards
>>> compatible with.
>>
>>
>> I do agree.
>>
>>
>>
>>>> E) Get rid of  ITK_USE_PORTABLE_ROUND and replace all calls to
>>>> vnl_math_rnd by calls to itk::Math::Round (or
>>>> itk::Math::RoundHalfIntegerBackwardCompatibility)
>>>>
>>>
>>>
>>> Yeap, this we should do before cutting the release.
>>> As soon as we fix the remining six failing tests.
>>
>>
>> Great!
>>
>>
>>
>>>> F) Bug fix that require a well defined half-integer rounding scheme
>>>> can now use itk::Math::RoundHalfIntegerUp or
>>>> itk::Math::RoundHalfIntegerToEven
>>>>
>>>>
>>>
>>>
>>> Yeap, this is how we got here.
>>> Michel and I were working on your patch for Bug #6558
>>> and Simon reminded us that this also required a consistent
>>> Round function to be available.
>>
>>
>> Thanks again for looking at this!
>>
>> Tom
>>
>