Avoid optimizing primitive #33 in vm.c (for VC++)

Jaco van der Merwe jaco.lists at telkomsa.net
Mon Aug 9 05:35:09 PDT 2004


Hi

Looking at the disassembly below it certainly looks like a global
optimization bug in VC++. I did some experimentation and found that the
following implementation of _primitive33() works without having to disable
any optimizations:

void _primitive33(struct Interpreter * interpreter, ObjectPointer *
arguments, unsigned long int argumentsSize, struct OopArray * optionals)
{
  signed long int y;
  signed long int x;
  x = ObjectPointer_asSmallInt( arguments[0] );
  y = ObjectPointer_asSmallInt( arguments[1] );
  PSInterpreter_stackPush_(interpreter, x < y ? CurrentMemory->TrueObject :
CurrentMemory->FalseObject);
}

Comparing this to the orignal version below

void _primitive33(struct Interpreter * interpreter, ObjectPointer *
arguments, unsigned long int argumentsSize, struct OopArray * optionals)
{
  ObjectPointer y;
  ObjectPointer x;
  x = arguments[0];
  y = arguments[1];
  PSInterpreter_stackPush_(interpreter, ObjectPointer_asSmallInt(x) <
ObjectPointer_asSmallInt(y)?CurrentMemory->TrueObject:CurrentMemory->FalseOb
ject);
}

it seems that the bug is caused by the interaction between the comparison
operator (<) with the inlining of the ObjectPointer_asSmallInt() function
calls. Taking out the latter into local variables somehow removes this
interaction and makes the optimization bug disappear.

I'm not familiar with the pidgin Slate for generating this primitive, so
maybe somebody familiar with this can update the pidgin code to generate the
function in the above form.

Reading the history of the Microsoft bug reports on global optimisation
bugs, it seems that they have been having problems with the interaction
between the shift operators and the comparison operators. By the way, these
bug reports are all for older versions of teh VC++ compilers, i.e., versions
5 and 6. One would expect that these bugs should have been fixed in the 7.0
and 7.1 versions, but maybe there are still some uhandled corner cases
lurking in their code somewhere.

Just to be on the safe side I would modify the form of ALL the pidgin code
that generates similar forms, i.e. inlined function calls (containing shift
operators) called around comparison operators, to first store the results of
the function calls in local variables and then use the comparison operator
after that on the local variables.

Regards
Jaco

----- Original Message ----- 
From: "Lendvai Attila" <Attila.Lendvai at netvisor.hu>
To: <slate at tunes.org>
Sent: Monday, August 09, 2004 12:08 PM
Subject: RE: Avoid optimizing primitive #33 in vm.c (for VC++)



hi!

Please find the disassembly below of the optimized/non-optimized
version. I've tried to surround the signed cast with an extra (), but
the same result

static INLINE signed long int ObjectPointer_asSmallInt(ObjectPointer
oop)
{
  return ((signed long int) oop) >> 1;
}

in the optimized version the rotation is done with an "shr" opcode,
which clears the MSB. in the non-optimized version, "sar" is used, which
behaves as expected...

there are other >> 1 operations all around in vm.c, but still turning
off global optimizations with the #pragma results in a working vm.

I'm downloading VC++ 2005 beta now and see what's in there?

- 101

MSDN jokes, the search itself:
http://search.microsoft.com/search/results.aspx?view=msdn&st=a&na=81&qu=
&qp=global+optimizations&qa=&qn=&c=10&s=1
some intresting pieces:
http://support.microsoft.com/default.aspx?scid=kb;en-us;250887
http://support.microsoft.com/default.aspx?scid=kb;en-us;113427



NON-OPTIMIZED:

void _primitive33(struct Interpreter * interpreter, ObjectPointer *
arguments, unsigned long int argumentsSize, struct OopArray * optionals)
{
00408B50  push        ebp
00408B51  mov         ebp,esp
00408B53  sub         esp,18h
  ObjectPointer x;
  ObjectPointer y;

  x = arguments[0];
00408B56  mov         eax,dword ptr [arguments]
00408B59  mov         ecx,dword ptr [eax]
00408B5B  mov         dword ptr [x],ecx
  y = arguments[1];
00408B5E  mov         edx,dword ptr [arguments]
00408B61  mov         eax,dword ptr [edx+4]
00408B64  mov         dword ptr [y],eax
  PSInterpreter_stackPush_(interpreter, ObjectPointer_asSmallInt(x) <
ObjectPointer_asSmallInt(y)?CurrentMemory->TrueObject:CurrentMemory->Fal
seObject);
00408B67  mov         ecx,dword ptr [x]
00408B6A  sar         ecx,1
00408B6C  mov         edx,dword ptr [y]
00408B6F  sar         edx,1
00408B71  cmp         ecx,edx
00408B73  jge         _primitive33+32h (408B82h)
00408B75  mov         eax,dword ptr [_CurrentMemory (411D60h)]
00408B7A  mov         ecx,dword ptr [eax+24h]
00408B7D  mov         dword ptr [ebp-18h],ecx
00408B80  jmp         _primitive33+3Eh (408B8Eh)
00408B82  mov         edx,dword ptr [_CurrentMemory (411D60h)]
00408B88  mov         eax,dword ptr [edx+28h]
00408B8B  mov         dword ptr [ebp-18h],eax
00408B8E  mov         ecx,dword ptr [ebp-18h]
00408B91  push        ecx
00408B92  mov         edx,dword ptr [interpreter]
00408B95  push        edx
00408B96  call        PSInterpreter_stackPush_ (406860h)
00408B9B  add         esp,8
}
00408B9E  mov         esp,ebp
00408BA0  pop         ebp
00408BA1  ret




OPTIMIZED:

void _primitive33(struct Interpreter * interpreter, ObjectPointer *
arguments, unsigned long int argumentsSize, struct OopArray * optionals)
{
  ObjectPointer x;
  ObjectPointer y;

  x = arguments[0];
  y = arguments[1];
  PSInterpreter_stackPush_(interpreter, ObjectPointer_asSmallInt(x) <
ObjectPointer_asSmallInt(y)?CurrentMemory->TrueObject:CurrentMemory->Fal
seObject);
004064E0  mov         eax,dword ptr [esp+8]
004064E4  mov         ecx,dword ptr [eax+4]
004064E7  mov         edx,dword ptr [eax]
004064E9  shr         ecx,1
004064EB  shr         edx,1
004064ED  cmp         edx,ecx
004064EF  push        esi
004064F0  push        edi
004064F1  jge         _primitive33+1Dh (4064FDh)
004064F3  mov         eax,dword ptr [_CurrentMemory (40FD60h)]
004064F8  mov         edi,dword ptr [eax+24h]
004064FB  jmp         _primitive33+26h (406506h)
004064FD  mov         ecx,dword ptr [_CurrentMemory (40FD60h)]
00406503  mov         edi,dword ptr [ecx+28h]
00406506  mov         esi,dword ptr [esp+0Ch]
0040650A  mov         edx,dword ptr [esi+24h]
0040650D  cmp         edx,dword ptr [esi+28h]
00406510  jne         _primitive33+3Bh (40651Bh)
00406512  push        esi
00406513  call        PSInterpreter_growStack (404D60h)
00406518  add         esp,4
0040651B  mov         eax,dword ptr [esi+24h]
0040651E  mov         ecx,dword ptr [esi+0Ch]
00406521  mov         dword ptr [ecx+eax*4+0Ch],edi
00406525  add         dword ptr [esi+24h],1
00406529  pop         edi
0040652A  pop         esi
}
0040652B  ret




:: -----Original Message-----
:: From: slate-bounces at tunes.org
:: [mailto:slate-bounces at tunes.org] On Behalf Of Lee Salzman
:: Sent: Monday, August 09, 2004 9:01 AM
:: To: Paul Dufresne
:: Cc: slate at tunes.org
:: Subject: Re: Avoid optimizing primitive #33 in vm.c (for VC++)
::
::
:: The primitive causing the problem is SmallInteger <
:: SmallInteger, not the Float one. Not sure why VC would do
:: this, though, other than that it
:: may just be a silly VC bug. Anyone has any ideas?
::
:: Lee
::
:: On Mon, Aug 09, 2004 at 06:11:45AM +0000, Paul Dufresne wrote:
:: > I said: "I think primitive 13 is this one:"
:: > but really meant primitive 33.
:: >
:: > Also, somehow I find it strange myself that the
:: > primitive causing problem be < for Floats,
:: > so it would be nice for someone to confirm.
:: >
:: > Finally, building bootstrap image have been done without a hitch.
:: > (Just hope the expression 'without a hitch' is Ok in english).
:: >
:: > --Paul
:: >
::
::




More information about the Slate mailing list