[Insight-developers] Rounding functions in itkMacro.h
Luis Ibanez
luis.ibanez at kitware.com
Sat May 16 12:09:46 EDT 2009
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