[Insight-developers] Rounding functions in itkMacro.h

Luis Ibanez luis.ibanez at kitware.com
Mon May 11 18:44:27 EDT 2009 

Hi Tom,

Setting  VNL_CONFIG_ENABLE_SSE2_ROUNDING to OFF
removes the declarations of the new rounding methods in
vnl_math.h.

I'm probably doing something wrong here.

I have generated a patch with the changes and uploaded it
to the bug tracker:

You will find it in:
    http://public.kitware.com/Bug/view.php?id=6558

is the patch named:

          PortableRound-May-11-2009.patch


If you have a chance,
could you help me find out what I'm missing ?


      Thanks


            Luis

---------------------------------------------------------------------
On Mon, May 11, 2009 at 6:34 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> 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
>>>
>>
>

More information about the Insight-developers mailing list