Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.
Close
From: Chris Sutcliffe <ir0nh34d@gm...>  20090717 02:00:53

>> I'm not familiar with the issue, but is this just a matter of a spurious >> warning, or is there actual incorrect code? I think we should avoid >> performance regressions whenever possible, particularly since users have >> recently indicated that the speed of these functions is important to them. > > I believe there could potentially be an issue with void pointers. > I've actually come up with another way to correct the issue in which I > used an intermediary type cast to char * (see the attached patch). > Using the test code, this patch achieves the same performance as > mingwrt3.15.2 with the test code I used from the mingwusers post. > > If no one has any objections to the patch, I'll commit this one. Does anyone know of some decent test code that will validate the changes I made? Chris  Chris Sutcliffe http://emergedesktop.org 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090713 18:05:14
Attachments:
mingwrt.patch

I'm updating mingwrt for GCC 4.4.0, and I'd appreciate some feedback: 1. To overcome the problem of strictaliasing in the match components of mingwrt, I've had to add some memcpy function calls (see the attached patch), are there any concerns in terms of performance? 2. Given that the binaries are going to be generated using GCC 4.4.0, does this warant a major version bump (i.e. to 4.0)? Once I've gone through the patch tracker, I'll upload a new version. Chris  Chris Sutcliffe http://emergedesktop.org 
From: Aaron W. LaFramboise <aaron77thyme@aa...>  20090714 04:24:29

Chris Sutcliffe wrote: > I'm updating mingwrt for GCC 4.4.0, and I'd appreciate some feedback: > > 1. To overcome the problem of strictaliasing in the match components > of mingwrt, I've had to add some memcpy function calls (see the > attached patch), are there any concerns in terms of performance? I'm going to guess there will be performance degradation, but I'm not sure. The best thing would be to run a quick performance comparison. Someone recently posted a nice little harness for this purpose to mingwusers regarding the GCC 4.4.0 complex regression. By the way, it's easier to review in email patches if you've used diff up and attached them as ContentDisposition inline. :) > 2. Given that the binaries are going to be generated using GCC 4.4.0, > does this warant a major version bump (i.e. to 4.0)? I don't think mingwrt has a version number policy; maybe you should write one (of the threesentence variety hopefully). The usual deal is that the major version is bumped for an ABI change, and compiling with GCC 4.4.0 instead of 3.4.5 should not be one. So, I would say 'no.' I would guess that there isn't a whole lot in mingwrt that would be particularly sensitive to the performance improvements in GCC 4.4, so this difference really should be fairly minor in any case. 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090714 11:36:53

Hey Aaron, >> 1. To overcome the problem of strictaliasing in the match components >> of mingwrt, I've had to add some memcpy function calls (see the >> attached patch), are there any concerns in terms of performance? > > I'm going to guess there will be performance degradation, but I'm not > sure. The best thing would be to run a quick performance comparison. > Someone recently posted a nice little harness for this purpose to > mingwusers regarding the GCC 4.4.0 complex regression. They seem basically consistant... mingwrt3.15.2 (no optimization): Time to create random variables: 1674 Time to calculate sum: 422 Time to calculate difference: 328 Time to calculate product: 657 Time to calculate quotient: 1095 Time to calculate square root: 4207 Time to calculate sin: 5678 Time to calculate cos: 5708 Time to calculate tan: 6147 Time to calculate exp: 3801 Time to calculate log: 5896 Time to calculate real part: 188 Time to calculate imaginary part: 172 Time to calculate norm: 3425 Time to calculate argument: 1517 mingwrtmemcpy (no optimization): Time to create random variables: 1276 Time to calculate sum: 389 Time to calculate difference: 342 Time to calculate product: 638 Time to calculate quotient: 1059 Time to calculate square root: 4092 Time to calculate sin: 5804 Time to calculate cos: 5726 Time to calculate tan: 6007 Time to calculate exp: 3812 Time to calculate log: 5275 Time to calculate real part: 171 Time to calculate imaginary part: 172 Time to calculate norm: 3563 Time to calculate argument: 1478 mingwrt3.15.2 (with O3): Time to create random variables: 1276 Time to calculate sum: 389 Time to calculate difference: 342 Time to calculate product: 638 Time to calculate quotient: 1059 Time to calculate square root: 4092 Time to calculate sin: 5804 Time to calculate cos: 5726 Time to calculate tan: 6007 Time to calculate exp: 3812 Time to calculate log: 5275 Time to calculate real part: 171 Time to calculate imaginary part: 172 Time to calculate norm: 3563 Time to calculate argument: 1478 mingwrtmemcpy (with O3): Time to create random variables: 1151 Time to calculate sum: 218 Time to calculate difference: 171 Time to calculate product: 311 Time to calculate quotient: 716 Time to calculate square root: 4202 Time to calculate sin: 5306 Time to calculate cos: 5322 Time to calculate tan: 5633 Time to calculate exp: 3363 Time to calculate log: 5733 Time to calculate real part: 125 Time to calculate imaginary part: 140 Time to calculate norm: 3521 Time to calculate argument: 1137 This being the case, are there any objections to committing the memcpy patch? > By the way, it's easier to review in email patches if you've used diff > up and attached them as ContentDisposition inline. :) Fair enough, I'll remember that for next time. :) >> 2. Given that the binaries are going to be generated using GCC 4.4.0, >> does this warant a major version bump (i.e. to 4.0)? > > I don't think mingwrt has a version number policy; maybe you should > write one (of the threesentence variety hopefully). I guess I should... Keith, I assume the wiki is the best place? > The usual deal is that the major version is bumped for an ABI change, > and compiling with GCC 4.4.0 instead of 3.4.5 should not be one. So, I > would say 'no.' Agreed, that's the policy I generally follow. I thought there was an ABI change between GCC 3.x and GCC 4.x? Cheers! Chris  Chris Sutcliffe http://emergedesktop.org 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090714 12:07:41

I just realized I reversed the times, below are corrected listings: > mingwrtmemcpy (no optimization): > > Time to create random variables: 1674 > Time to calculate sum: 422 > Time to calculate difference: 328 > Time to calculate product: 657 > Time to calculate quotient: 1095 > Time to calculate square root: 4207 > Time to calculate sin: 5678 > Time to calculate cos: 5708 > Time to calculate tan: 6147 > Time to calculate exp: 3801 > Time to calculate log: 5896 > Time to calculate real part: 188 > Time to calculate imaginary part: 172 > Time to calculate norm: 3425 > Time to calculate argument: 1517 > > mingwrt3.15.2 (no optimization): > > Time to create random variables: 1276 > Time to calculate sum: 389 > Time to calculate difference: 342 > Time to calculate product: 638 > Time to calculate quotient: 1059 > Time to calculate square root: 4092 > Time to calculate sin: 5804 > Time to calculate cos: 5726 > Time to calculate tan: 6007 > Time to calculate exp: 3812 > Time to calculate log: 5275 > Time to calculate real part: 171 > Time to calculate imaginary part: 172 > Time to calculate norm: 3563 > Time to calculate argument: 1478 > > mingwrtmemcpy (with O3): > > Time to create random variables: 1276 > Time to calculate sum: 389 > Time to calculate difference: 342 > Time to calculate product: 638 > Time to calculate quotient: 1059 > Time to calculate square root: 4092 > Time to calculate sin: 5804 > Time to calculate cos: 5726 > Time to calculate tan: 6007 > Time to calculate exp: 3812 > Time to calculate log: 5275 > Time to calculate real part: 171 > Time to calculate imaginary part: 172 > Time to calculate norm: 3563 > Time to calculate argument: 1478 > > mingwrt3.15.2 (with O3): > > Time to create random variables: 1151 > Time to calculate sum: 218 > Time to calculate difference: 171 > Time to calculate product: 311 > Time to calculate quotient: 716 > Time to calculate square root: 4202 > Time to calculate sin: 5306 > Time to calculate cos: 5322 > Time to calculate tan: 5633 > Time to calculate exp: 3363 > Time to calculate log: 5733 > Time to calculate real part: 125 > Time to calculate imaginary part: 140 > Time to calculate norm: 3521 > Time to calculate argument: 1137 Now the numbers make a little more sense, so there is performance impact. Is it significant enough to warrant looking at an alternate implementation? I tried using the 'union' method to get around the strictaliasing issue (I read about it on Google), but I it didn't get rid of the warnings for me. I'll investigate it further. Chris  Chris Sutcliffe http://emergedesktop.org 
From: Keith Marshall <keithmarshall@us...>  20090714 17:36:19

On Tuesday 14 July 2009 12:36:46 Chris Sutcliffe wrote: > > I don't think mingwrt has a version number policy; maybe you > > should write one (of the threesentence variety hopefully). > > I guess I should... Keith, I assume the wiki is the best place? Yes, but as a static page, rather than as a regular wiki page please. Static pages have more restrictive modification rights than regular wiki pages, and we don't want to give every man and his dog rights to modify policy statements.  Regards, Keith. 
From: Aaron W. LaFramboise <aaron77thyme@aa...>  20090715 01:38:26

Chris Sutcliffe wrote: > Now the numbers make a little more sense, so there is performance > impact. Is it significant enough to warrant looking at an alternate > implementation? I'm not familiar with the issue, but is this just a matter of a spurious warning, or is there actual incorrect code? I think we should avoid performance regressions whenever possible, particularly since users have recently indicated that the speed of these functions is important to them. 
From: Aaron W. LaFramboise <aaron77thyme@aa...>  20090715 01:39:54

Chris Sutcliffe wrote: >> The usual deal is that the major version is bumped for an ABI change, >> and compiling with GCC 4.4.0 instead of 3.4.5 should not be one. So, I >> would say 'no.' > > Agreed, that's the policy I generally follow. I thought there was an > ABI change between GCC 3.x and GCC 4.x? As far as I know, there are no ABI changes for C for the core language except when fexceptions is given. Are we compiling mingwrt that, by any chance? This would only affect the ability to throw through qsort() and similar. 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090715 02:10:05

> As far as I know, there are no ABI changes for C for the core language > except when fexceptions is given. Are we compiling mingwrt that, by > any chance? This would only affect the ability to throw through qsort() > and similar. Nope, fexceptions is not used when compiling mingwrt. As you pointed out, this runtime should be compatible with GCC 3.x and 4.x. Thank you for the clarification. Chris  Chris Sutcliffe http://emergedesktop.org 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090715 02:08:12
Attachments:
mingwrtmath.patch

> I'm not familiar with the issue, but is this just a matter of a spurious > warning, or is there actual incorrect code? I think we should avoid > performance regressions whenever possible, particularly since users have > recently indicated that the speed of these functions is important to them. I believe there could potentially be an issue with void pointers. I've actually come up with another way to correct the issue in which I used an intermediary type cast to char * (see the attached patch). Using the test code, this patch achieves the same performance as mingwrt3.15.2 with the test code I used from the mingwusers post. If no one has any objections to the patch, I'll commit this one. Cheers! Chris  Chris Sutcliffe http://emergedesktop.org 
From: Chris Sutcliffe <ir0nh34d@gm...>  20090717 02:00:53

>> I'm not familiar with the issue, but is this just a matter of a spurious >> warning, or is there actual incorrect code? I think we should avoid >> performance regressions whenever possible, particularly since users have >> recently indicated that the speed of these functions is important to them. > > I believe there could potentially be an issue with void pointers. > I've actually come up with another way to correct the issue in which I > used an intermediary type cast to char * (see the attached patch). > Using the test code, this patch achieves the same performance as > mingwrt3.15.2 with the test code I used from the mingwusers post. > > If no one has any objections to the patch, I'll commit this one. Does anyone know of some decent test code that will validate the changes I made? Chris  Chris Sutcliffe http://emergedesktop.org 