[Insight-developers] Rounding functions in itkMacro.h

Tom Vercauteren tom.vercauteren at m4x.org
Mon May 11 19:16:33 EDT 2009 

Luis,

Can you provide some information on your configuration (OS, compiler,
cpu)? Also did you get a compilation error or a link error?

One potential thing to look at is whether your machine actually
supports sse2 and whether vxl correctly detects it. It looks like I
missed a patch to the sse2 detection in the list I gave you:
http://vxl.svn.sourceforge.net/viewvc/vxl/trunk/config/cmake/config/CMakeLists.txt?r1=23457&r2=23456&pathrev=23457
http://vxl.svn.sourceforge.net/viewvc/vxl/trunk/config/cmake/config/vxl_platform_tests.cxx?r1=23457&r2=23456&pathrev=23457

No matter this patch, setting  VNL_CONFIG_ENABLE_SSE2_ROUNDING to OFF
should definitely not remove the declarations of the new rounding
methods. It should simply fallback to another implementation than the
SSE2 one (gcc-specific or msvc-specific or vanilla c).

Since I don't have access to my computer right now, it's a bit
difficult for me to provide you more information right now.

Tom

On Tue, May 12, 2009 at 00:44, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> 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