Jump to content

  • Log In with Google      Sign In   
  • Create Account

- - - - -

[PATCH] floating point comparisons involving infinity


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
2 replies to this topic

#1 GGLucas   Members   -  Reputation: 140

Like
0Likes
Like

Posted 11 September 2012 - 07:30 PM

Hello! I work with Andrew (Thy Reaper), and have reported a couple of bugs through him before. This one seemed easy enough to fix so I decided to submit a patch instead.

I bound float-infinity as a constant global to the scripts, and found out that because of the way AS does its comparisons, INFINITY != INFINITY, which is counter to the behaviour of infinities in C/C++. If that's intended feel free to ignore the patch, but I can't think of a reason why it would be.

To me, at least, it looks like the way the comparisons are done is a premature optimization. The compiler can figure out how to optimize it better, and from what I can tell from a trivial decompile, it might very well do better on the plain version that I changed it to (two compares versus a subtract and two compares). That is, of course, all highly machine- and compiler-specific speculation, but in general I think it's better to leave it to the C++ compiler to decide what's best.

[source lang="cpp"]Index: sdk/angelscript/source/as_context.cpp===================================================================--- sdk/angelscript/source/as_context.cpp (revision 1413)+++ sdk/angelscript/source/as_context.cpp (working copy)@@ -2149,10 +2149,11 @@ // Comparisons case asBC_CMPd: {- double dbl = *(double*)(l_fp - asBC_SWORDARG0(l_bc)) - *(double*)(l_fp - asBC_SWORDARG1(l_bc));- if( dbl == 0 ) *(int*)&m_regs.valueRegister = 0;- else if( dbl < 0 ) *(int*)&m_regs.valueRegister = -1;- else *(int*)&m_regs.valueRegister = 1;+ double dbl1 = *(double*)(l_fp - asBC_SWORDARG0(l_bc));+ double dbl2 = *(double*)(l_fp - asBC_SWORDARG1(l_bc));+ if( dbl1 == dbl2 ) *(int*)&m_regs.valueRegister = 0;+ else if( dbl1 < dbl2 ) *(int*)&m_regs.valueRegister = -1;+ else *(int*)&m_regs.valueRegister = 1; l_bc += 2; } break;@@ -2170,20 +2171,22 @@ case asBC_CMPf: {- float f = *(float*)(l_fp - asBC_SWORDARG0(l_bc)) - *(float*)(l_fp - asBC_SWORDARG1(l_bc));- if( f == 0 ) *(int*)&m_regs.valueRegister = 0;- else if( f < 0 ) *(int*)&m_regs.valueRegister = -1;- else *(int*)&m_regs.valueRegister = 1;+ float f1 = *(float*)(l_fp - asBC_SWORDARG0(l_bc));+ float f2 = *(float*)(l_fp - asBC_SWORDARG1(l_bc));+ if( f1 == f2 ) *(int*)&m_regs.valueRegister = 0;+ else if( f1 < f2 ) *(int*)&m_regs.valueRegister = -1;+ else *(int*)&m_regs.valueRegister = 1; l_bc += 2; } break; case asBC_CMPi: {- int i = *(int*)(l_fp - asBC_SWORDARG0(l_bc)) - *(int*)(l_fp - asBC_SWORDARG1(l_bc));- if( i == 0 ) *(int*)&m_regs.valueRegister = 0;- else if( i < 0 ) *(int*)&m_regs.valueRegister = -1;- else *(int*)&m_regs.valueRegister = 1;+ int i1 = *(int*)(l_fp - asBC_SWORDARG0(l_bc));+ int i2 = *(int*)(l_fp - asBC_SWORDARG1(l_bc));+ if( i1 == i2 ) *(int*)&m_regs.valueRegister = 0;+ else if( i1 < i2 ) *(int*)&m_regs.valueRegister = -1;+ else *(int*)&m_regs.valueRegister = 1; l_bc += 2; } break;@@ -2192,20 +2195,22 @@ // Comparisons with constant value case asBC_CMPIi: {- int i = *(int*)(l_fp - asBC_SWORDARG0(l_bc)) - asBC_INTARG(l_bc);- if( i == 0 ) *(int*)&m_regs.valueRegister = 0;- else if( i < 0 ) *(int*)&m_regs.valueRegister = -1;- else *(int*)&m_regs.valueRegister = 1;+ int i1 = *(int*)(l_fp - asBC_SWORDARG0(l_bc));+ int i2 = asBC_INTARG(l_bc);+ if( i1 == i2 ) *(int*)&m_regs.valueRegister = 0;+ else if( i1 < i2 ) *(int*)&m_regs.valueRegister = -1;+ else *(int*)&m_regs.valueRegister = 1; l_bc += 2; } break; case asBC_CMPIf: {- float f = *(float*)(l_fp - asBC_SWORDARG0(l_bc)) - asBC_FLOATARG(l_bc);- if( f == 0 ) *(int*)&m_regs.valueRegister = 0;- else if( f < 0 ) *(int*)&m_regs.valueRegister = -1;- else *(int*)&m_regs.valueRegister = 1;+ float f1 = *(float*)(l_fp - asBC_SWORDARG0(l_bc));+ float f2 = asBC_FLOATARG(l_bc);+ if( f1 == f2 ) *(int*)&m_regs.valueRegister = 0;+ else if( f1 < f2 ) *(int*)&m_regs.valueRegister = -1;+ else *(int*)&m_regs.valueRegister = 1; l_bc += 2; } break;[/source]


PS. If you feel like setting one up, github is actually quite amazing for open source projects - it makes contributing a much smoother process on both ends. As a contributor I could clone the official repository and use version control myself instead of having to submit loose patches, and as a developer all you need to do is look at the pull request, inspect the patches and press the nice accept button instead of having to apply a bunch of separate patches and create commits. Just a suggestion.

Sponsor:

#2 Andreas Jonsson   Moderators   -  Reputation: 3356

Like
0Likes
Like

Posted 12 September 2012 - 08:39 AM

This was definitely not intentional from my part, nor was it an attempt to optimize the code.

Thanks for the patch. I'll have it applied in the next check-in.


Maybe I'll look into using github in the future, others have made the same suggestion before. It's just that I'm comfortable with how things are now and don't really feel a need to change it myself. The act of manually merging the patches I receive gives me a better chance to actually analyse what is being modified, to think about the impacts it will have.
AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

#3 Andreas Jonsson   Moderators   -  Reputation: 3356

Like
0Likes
Like

Posted 12 September 2012 - 03:36 PM

I've checked in the patch to revision 1414.

Thanks,
Andreas
AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS