[Insight-developers] Rounding functions in itkMacro.h
Tom Vercauteren
tom.vercauteren at m4x.org
Mon May 18 04:51:17 EDT 2009
Thanks Luis,
Looking at these modifications:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkNearestNeighborExtrapolateImageFunction.h?root=Insight&r1=1.6&r2=1.7&sortby=date
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkMacro.h?root=Insight&r1=1.94&r2=1.95&sortby=date
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&r1=1.61&r2=1.62&sortby=date
it looks like some code may be cleanup a bit now. itk::Math::Round
could be used without checking enclosing it in a ifdef block.
I also had a quick look at the dashboard failures (previous ones as it
is a little early for the current ones). I don't understand some of
them. For example:
http://www.cdash.org/CDash/viewBuildError.php?buildid=334444
leads me to think that -msse2 is required but this flag does not seem
to appear in ITK_REQUIRED_CXX_FLAGS for this build:
http://www.cdash.org/CDash/testDetails.php?test=23205678&build=334444
I just tested a gcc 3.4 build on my local machine:
CC="gcc-3.4" CXX="g++-3.4" cmake
-DVNL_CONFIG_ENABLE_SSE2_ROUNDING=ON <path_to_itk>
and it works fine. I do see the -msse2 flag in ITK_REQUIRED_CXX_FLAGS
as shown in ITKConfig.cmake.
Similarly
CC="gcc-3.4" CXX="g++-3.4" cmake <path_to_itk>
also works fine and as expected, -msse2 is not in
ITK_REQUIRED_CXX_FLAGS since VNL_CONFIG_ENABLE_SSE2_ROUNDING is
undefined.
I'll try to take some time later today to look at the current
dashboard failures.
Tom
On Sat, May 16, 2009 at 18:09, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> Hi Tom,
>
> The patch has been committed.
>
> I'll track the Dashboard...
>
> Luis
>
> -----------------------------------------------------------------
> On Sat, May 16, 2009 at 9:09 AM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>> Hi Tom
>>
>> Thanks for uploading the new version of the patch.
>>
>> I'm first checking that everything will work if we have
>> ITK_USE_PORTABLE_ROUND set to OFF. Then we
>> can enable a couple of machines to deal with the ON case.
>>
>> So, for the OFF case:
>>
>> A) After applying it (locally) three test were failing.
>>
>> 1) itkInterpolateTest
>> 2) ResampleImageFilter3Test1
>> 3) ResampleImageFilter9Test
>>
>> The first one has been fixed. It had hand-coded
>> values that implied the rounding from continuous
>> indexes to integers.
>>
>> The two others seems to require new baseline images,
>> although it is strange that they are the only ones failing.
>>
>> B) In order to use ITK from an external project I had to change
>>
>> SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -msse2")
>>
>> to
>>
>> SET(ITK_REQUIRED_CXX_FLAGS "${ITK_REQUIRED_CXX_FLAGS} -msse2")
>>
>>
>> I'm looking at the two remaining failing tests,
>> and plan to commit your patch by the end of the morning.
>>
>>
>>
>> Thanks
>>
>>
>> Luis
>>
>>
>>
>> -------------------------------------------------------------------------------------------
>> On Fri, May 15, 2009 at 1:05 PM, Tom Vercauteren
>> <tom.vercauteren at m4x.org> wrote:
>>> Hi Luis,
>>>
>>> I have posted a new version of the portable round patch on the bug tracker:
>>> http://public.kitware.com/Bug/view.php?id=6558
>>> http://public.kitware.com/Bug/file_download.php?file_id=2254&type=bug
>>>
>>> It fixes the include problem that I mentioned and also modifies two things:
>>> 1) VNL_CONFIG_ENABLE_SSE2_ROUNDING is turned ON by default only if the
>>> compiler supports it natively (i.e. without the -msse2 flag)
>>> 2) VNL_CONFIG_ENABLE_SSE2_ROUNDING is exported in ITKConfig.cmake.in
>>>
>>> Both modification were done to improve the user experience when using
>>> ITK from an external project.
>>>
>>> Regarding point 1:
>>> My previous patch was setting VNL_CONFIG_ENABLE_SSE2_ROUNDING to ON by
>>> default if the compiler supported sse2 natively OR if it supported it
>>> with the -msse2 flag. If -msse2 was required, cmake was adding this
>>> flag by default.
>>> This is not very nice to the user as it requires him by default to
>>> also use this flag in its external project even though he didn't
>>> explicitly ask to add sse2 support.
>>>
>>> Regarding point 2:
>>> If the user explicitly chose to turn VNL_CONFIG_ENABLE_SSE2_ROUNDING
>>> and his compiler needs the -mss2 flag for that, ITK will use it.
>>> That's fine. Now if ITK is included in an external project, this
>>> project will also require the -msse2 flag. Adding
>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING to ITKConfig.cmake allows the
>>> external project to know whether it needs to use -msse2 by default or
>>> not.
>>>
>>> Cheers,
>>> Tom
>>>
>>>
>>> On Wed, May 13, 2009 at 01:05, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>>>
>>>> Tom,
>>>>
>>>> Thanks for catching the problem.
>>>>
>>>> I'll try running another Experimental tonight.
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>> ------------------------
>>>> Tom Vercauteren wrote:
>>>>>
>>>>> Oups, in my previous email
>>>>> "I see that vnl_math.h is only included if ITK_USE_PORTABLE_ROUND is ON."
>>>>> should have been
>>>>> "I see that vnl_math.h is only included if ITK_USE_PORTABLE_ROUND is
>>>>> OFF or not defined."
>>>>>
>>>>> Tom
>>>>>
>>>>> On Tue, May 12, 2009 at 23:26, Tom Vercauteren <tom.vercauteren at m4x.org>
>>>>> wrote:
>>>>>
>>>>>> Hi Luis,
>>>>>>
>>>>>> I think I got the culprit. It didn't make sense to me how
>>>>>> vnl_math_rnd_halfintup could be undeclared. I am using ubuntu 9.04
>>>>>> with gcc 4.3 and every thing works out fine. Se for example this debug
>>>>>> build with sse2 rounding:
>>>>>> http://www.cdash.org/CDash/buildSummary.php?buildid=331407
>>>>>> http://www.cdash.org/CDash/testDetails.php?test=22870870&build=331407
>>>>>>
>>>>>> The only possible reason for a mising vnl_math_rnd_halfintup is a
>>>>>> missing or outdated vnl_math.h. Looking back at the code
>>>>>>
>>>>>> http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/Common/itkMacro.h?root=Insight&view=markup
>>>>>> I see that vnl_math.h is only included if ITK_USE_PORTABLE_ROUND is
>>>>>> ON. I wasn't using this variable...
>>>>>>
>>>>>> Could you please try and recompile by changing
>>>>>> #ifndef ITK_USE_PORTABLE_ROUND
>>>>>> #include "vnl/vnl_math.h"
>>>>>> #endif
>>>>>> by
>>>>>> #include "vnl/vnl_math.h"
>>>>>> in
>>>>>> itkMacro.h
>>>>>> ?
>>>>>>
>>>>>> Sorry for not giving you a patch but I don't have a development machine
>>>>>> at hand.
>>>>>>
>>>>>> Also, I saw from your previous email that you had set
>>>>>> VNL_CONFIG_ENABLE_SSE2 to ON. This is not used by the sse2 rounding
>>>>>> functions which only relies on VNL_CONFIG_ENABLE_SSE2_ROUNDING. Since
>>>>>> VXL developers aresaying that VNL_CONFIG_ENABLE_SSE2 is unstable, I
>>>>>> wouldn't recommend turning this option on.
>>>>>>
>>>>>> Hope this helps,
>>>>>> Tom
>>>>>>
>>>>>>
>>>>>> On Tue, May 12, 2009 at 18:14, Luis Ibanez <luis.ibanez at kitware.com>
>>>>>> wrote:
>>>>>>
>>>>>>> HI Tom,
>>>>>>>
>>>>>>> Thanks for the new patch.
>>>>>>>
>>>>>>> Unfortunately it is not compiling for me.
>>>>>>>
>>>>>>>
>>>>>>> Here is what I got:
>>>>>>>
>>>>>>>
>>>>>>> [ 41%] Building CXX object
>>>>>>> Code/Common/CMakeFiles/ITKCommon.dir/itkBarrier.o
>>>>>>> In file included from
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkTimeStamp.h:23,
>>>>>>> from
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkLightObject.h:21,
>>>>>>> from /home/ibanez/src/Insight/Code/Common/itkBarrier.h:20,
>>>>>>> from
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkBarrier.cxx:17:
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h: In function ‘int
>>>>>>> itk::Math::RoundHalfIntegerUp(float)’:
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h:979: error:
>>>>>>> ‘vnl_math_rnd_halfintup’ was not declared in this scope
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h: In function ‘int
>>>>>>> itk::Math::RoundHalfIntegerUp(double)’:
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h:980: error:
>>>>>>> ‘vnl_math_rnd_halfintup’ was not declared in this scope
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h: In function ‘int
>>>>>>> itk::Math::RoundHalfIntegerToEven(float)’:
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h:981: error:
>>>>>>> ‘vnl_math_rnd_halfinttoeven’ was not declared in this scope
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h: In function ‘int
>>>>>>> itk::Math::RoundHalfIntegerToEven(double)’:
>>>>>>> /home/ibanez/src/Insight/Code/Common/itkMacro.h:982: error:
>>>>>>> ‘vnl_math_rnd_halfinttoeven’ was not declared in this scope
>>>>>>> make[2]: *** [Code/Common/CMakeFiles/ITKCommon.dir/itkBarrier.o] Error 1
>>>>>>> make[1]: *** [Code/Common/CMakeFiles/ITKCommon.dir/all] Error 2
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> and the SSE flags in my build are ON
>>>>>>>
>>>>>>> grep ENABLE_SSE MakeCache.txt
>>>>>>>
>>>>>>> VNL_CONFIG_ENABLE_SSE2:BOOL=ON
>>>>>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING:BOOL=ON
>>>>>>> //Advanced flag for variable: VNL_CONFIG_ENABLE_SSE2
>>>>>>> VNL_CONFIG_ENABLE_SSE2-ADVANCED:INTERNAL=1
>>>>>>> //Modified flag for variable: VNL_CONFIG_ENABLE_SSE2
>>>>>>> VNL_CONFIG_ENABLE_SSE2-MODIFIED:INTERNAL=1
>>>>>>> //Advanced flag for variable: VNL_CONFIG_ENABLE_SSE2_ROUNDING
>>>>>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING-ADVANCED:INTERNAL=1
>>>>>>> //Modified flag for variable: VNL_CONFIG_ENABLE_SSE2_ROUNDING
>>>>>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING-MODIFIED:INTERNAL=1
>>>>>>>
>>>>>>>
>>>>>>> BTW: This is a Debug build in gcc 4.3.2, Linux Ubuntu 8.10.
>>>>>>>
>>>>>>>
>>>>>>> Luis
>>>>>>>
>>>>>>>
>>>>>>> =========================================
>>>>>>> On Tue, May 12, 2009 at 8:48 AM, Tom Vercauteren
>>>>>>> <tom.vercauteren at m4x.org> wrote:
>>>>>>>
>>>>>>>> Luis,
>>>>>>>>
>>>>>>>> I have put an updated patch on the bug tracker:
>>>>>>>> http://public.kitware.com/Bug/file_download.php?file_id=2236&type=bug
>>>>>>>>
>>>>>>>> In addition to the changes I mentioned earlier, it includes some small
>>>>>>>> modifications to some CMakeLists.txt in order to ease turning on SSE2
>>>>>>>> rounding.
>>>>>>>> cmake -DVNL_CONFIG_ENABLE_SSE2_ROUNDING=ON <path_to_itk>
>>>>>>>> and
>>>>>>>> ccmake <path_to_itk>
>>>>>>>> switch VNL_CONFIG_ENABLE_SSE2_ROUNDING to on
>>>>>>>> now also works with gcc
>>>>>>>>
>>>>>>>> Let me know if it works for you.
>>>>>>>>
>>>>>>>> Tom
>>>>>>>>
>>>>>>>> On Tue, May 12, 2009 at 12:43, Tom Vercauteren
>>>>>>>> <tom.vercauteren at m4x.org> wrote:
>>>>>>>>
>>>>>>>>> Hi Luis,
>>>>>>>>>
>>>>>>>>> I have almost the same system so this will ease the process.
>>>>>>>>>
>>>>>>>>> The problem is indeed related to the -msse2 flag. Running
>>>>>>>>> cmake -DCMAKE_C_FLAGS="-msse2" -DCMAKE_CXX_FLAGS="-msse2"
>>>>>>>>> -DVNL_CONFIG_ENABLE_SSE2_ROUNDING=ON <path_to_itk>
>>>>>>>>> works correctly.
>>>>>>>>>
>>>>>>>>> So it looks like the cmake magic used by vxl to detect and add sse2
>>>>>>>>> support is wrong. Actually running
>>>>>>>>> cmake -DVNL_CONFIG_ENABLE_SSE2_ROUNDING=ON <path_to_itk>
>>>>>>>>> leads to a cmake error.
>>>>>>>>>
>>>>>>>>> This is not specific to the patched itk version as the same behavior
>>>>>>>>> is observed on a pristine checkout of the vxl svn trunk.
>>>>>>>>>
>>>>>>>>> There are at least two errors I can see from the cmake scripts in vxl
>>>>>>>>> svn trunk when VNL_CONFIG_ENABLE_SSE2_ROUNDING is ON.
>>>>>>>>>
>>>>>>>>> 1) ADD_DEFINITIONS( -msse2 ) is called after vxl tries to detect
>>>>>>>>> whether it has sse2 support. However this flag is required for gcc
>>>>>>>>> (4.3) to find emmintrin.h and use the sse2 intrinsics.
>>>>>>>>>
>>>>>>>>> 2) Even if ADD_DEFINITIONS( -msse2 ) was called prior to
>>>>>>>>> PERFORM_CHECK_HEADER(emmintrin.h VXL_HAS_EMMINTRIN_H)
>>>>>>>>> and
>>>>>>>>> PERFORM_CMAKE_TEST_RUN(vxl_platform_tests.cxx
>>>>>>>>> VXL_HAS_SSE2_HARDWARE_SUPPORT)
>>>>>>>>> it wouldn't make a difference since PERFORM_CHECK_HEADER and
>>>>>>>>> PERFORM_CMAKE_TEST_RUN do not rely on such ADD_DEFINITIONS.
>>>>>>>>>
>>>>>>>>> I am not sure how to fix this right now as I am not a cmake expert.
>>>>>>>>> Any help would be appreciated.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Note this this issue does not affect the case when
>>>>>>>>> VNL_CONFIG_ENABLE_SSE2_ROUNDING is OFF. In such a case, on your
>>>>>>>>> system, the implementation will fallback to the gcc assembly calls,
>>>>>>>>> e.g.
>>>>>>>>> inline int vnl_math_rnd_halfinttoeven(float x) { int r; __asm__
>>>>>>>>> __volatile__ ("fistpl %0" : "=m"(r) : "t"(x) : "st"); return r; }
>>>>>>>>>
>>>>>>>>> I'll report back if I find a patch for the cmake scripts that I find
>>>>>>>>> reasonable.
>>>>>>>>>
>>>>>>>>> Tom
>>>>>>>>>
>>>>>>>>> On Tue, May 12, 2009 at 04:27, Luis Ibanez <luis.ibanez at kitware.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Tom,
>>>>>>>>>>
>>>>>>>>>> I'm testing this on a Laptop:
>>>>>>>>>>
>>>>>>>>>> Linux Ubuntu 8.10
>>>>>>>>>> gcc 4.3.2
>>>>>>>>>> Intel(R) Core(TM)2 Duo CPU T9600 @ 2.80GHz
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The error was a link error.
>>>>>>>>>> Missing symbols.
>>>>>>>>>>
>>>>>>>>>> lshw returns the following data from the CPU:
>>>>>>>>>>
>>>>>>>>>> *-cpu
>>>>>>>>>> description: CPU
>>>>>>>>>> product: Intel(R) Core(TM)2 Duo CPU T9600 @ 2.80GHz
>>>>>>>>>> vendor: Intel Corp.
>>>>>>>>>> physical id: 400
>>>>>>>>>> bus info: cpu at 0
>>>>>>>>>> version: 6.7.6
>>>>>>>>>> slot: Microprocessor
>>>>>>>>>> size: 2801MHz
>>>>>>>>>> capacity: 2801MHz
>>>>>>>>>> width: 64 bits
>>>>>>>>>> clock: 266MHz
>>>>>>>>>> capabilities: fpu fpu_exception wp vme de pse tsc msr pae
>>>>>>>>>> mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx
>>>>>>>>>> fxsr
>>>>>>>>>> sse sse2 ss ht tm pbe nx x86-64 constant_tsc arch_perfmon pebs bts
>>>>>>>>>> pni
>>>>>>>>>> monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr sse4_1 lahf_lm cpufreq
>>>>>>>>>> configuration: id=0
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>>
>>>>>>>>>> Luis
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ------------------------------
>>>>>>>>>> On Mon, May 11, 2009 at 7:16 PM, Tom Vercauteren
>>>>>>>>>> <tom.vercauteren at m4x.org> wrote:
>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the Insight-developers
mailing list