[commit] r2463 - CogVM source as per VMMaker.oscog-eem.105. Fix signed32BitValueOf for most neg-

On Thu, Jul 21, 2011 at 07:08:18AM -0400, David T. Lewis wrote:
>
> Commit message blocked due to size, forwarding trimmed version:
>
> Author: eliot
> Date: 2011-07-18 17:35:51 -0700 (Mon, 18 Jul 2011)
> New Revision: 2463

<snip>

> Log:
> CogVM source as per VMMaker.oscog-eem.105.  Fix signed32BitValueOf for most neg-
> ative value C compiler mis-optimization.  Speed up primitiveFail using ! trick.
> Add multi-threaded sources to tree (won't build yet due to issue in ia32abicc.c)
> Upgrade nscogsrc/plugins to official versions.

Hi Eliot,

Can you say what the issue was with signed32BitValueOf? I can
see the changes in InterpreterPrimitives>>signed32BitValueOf:
but I'm not clear on whether this is something that affects
Alien, or if it is something that has been causing problems
more generally but went unnoticed. Also, I'd like to document
this with a unit test, so if you can suggest a code snippet
that would be great.

TIA,
Dave

Hi Eliot:

On 21/07/11 19:35, Eliot Miranda wrote:

On Thu, Jul 21, 2011 at 4:30 AM, David T. Lewis <[hidden email] <mailto:[hidden email]>> wrote:



   Hi Eliot,

   Can you say what the issue was with signed32BitValueOf? I can
   see the changes in InterpreterPrimitives>>signed32BitValueOf:
   but I'm not clear on whether this is something that affects
   Alien, or if it is something that has been causing problems
   more generally but went unnoticed. Also, I'd like to document
   this with a unit test, so if you can suggest a code snippet
   that would be great.


The unit test is in the lien tests and is the attempt to assign max neg int (-2^31) through signedLongAt:put:.  The problem is due to a pervasive C compiler bug with optimization.

Maybe I misread this blog post: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html (*Signed integer overflow:)*
but from that I would conclude that it is not a compiler bug, but undefined behavior in C instead. Thus, GCC and ICC are just doing there job in terms of what they are allowed to do by the spec.

for (i = 0; i <= N; ++i) { ... }

Best regards
Stefan

On Thu, Jul 21, 2011 at 03:24:08PM -0700, Eliot Miranda wrote:
>
> On Thu, Jul 21, 2011 at 3:17 PM, Stefan Marr <[hidden email]> wrote:
>
> >
> > Hi Eliot:
> >
> > On 21/07/11 19:35, Eliot Miranda wrote:
> >
> >
> >> On Thu, Jul 21, 2011 at 4:30 AM, David T. Lewis <[hidden email]<mailto:
> >> [hidden email]>> wrote:
> >>
> >>
> >>
> >>    Hi Eliot,
> >>
> >>    Can you say what the issue was with signed32BitValueOf? I can
> >>    see the changes in InterpreterPrimitives>>**signed32BitValueOf:
> >>    but I'm not clear on whether this is something that affects
> >>    Alien, or if it is something that has been causing problems
> >>    more generally but went unnoticed. Also, I'd like to document
> >>    this with a unit test, so if you can suggest a code snippet
> >>    that would be great.
> >>
> >>
> >> The unit test is in the lien tests and is the attempt to assign max neg
> >> int (-2^31) through signedLongAt:put:.  The problem is due to a pervasive C
> >> compiler bug with optimization.
> >>
> > Maybe I misread this blog post: http://blog.llvm.org/2011/05/**

> > what-every-c-programmer-**should-know.html<http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html>(*Signed integer overflow:)*

> > but from that I would conclude that it is not a compiler bug, but undefined
> > behavior in C instead. Thus, GCC and ICC are just doing there job in terms
> > of what they are allowed to do by the spec.
> >
>
> Thanks, Stefan.  I hadn't known this (or had forgotten):
>
> *Signed integer overflow:* If arithmetic on an 'int' type (for example)
> overflows, the result is undefined. One example is that "INT_MAX+1" is not
> guaranteed to be INT_MIN. This behavior enables certain classes of
> optimizations that are important for some code. For example, knowing that
> INT_MAX+1 is undefined allows optimizing "X+1 > X" to "true". Knowing the
> multiplication "cannot" overflow (because doing so would be undefined)
> allows optimizing "X*2/2" to "X". While these may seem trivial, these sorts
> of things are commonly exposed by inlining and macro expansion. A more
> important optimization that this allows is for "<=" loops like this:
>
> for (i = 0; i <= N; ++i) { ... }
>
>
> In this loop, the compiler can assume that the loop will iterate exactly N+1
> times if "i" is undefined on overflow, which allows a broad range of loop
> optimizations to kick in. On the other hand, if the variable is defined to
> wrap around on overflow, then the compiler must assume that the loop is
> possibly infinite (which happens if N is INT_MAX) - which then disables
> these important loop optimizations. This particularly affects 64-bit
> platforms since so much code uses "int" as induction variables.
>
> Makes evil sense :)

The issue does not exist in VMMaker trunk, which uses a simpler and
presumably faster implementation.

There is no need for the "value - 1"
logic, hence no need for a workaround in the special case of the max
negative value. The relevant methods are also tested on 32/64 host
and image combinations.

Here is the logic from #signed32BitValueOf: with the special case
handling removed entirely:

   "Fail if value exceeds range of a 32-bit two's-complement signed integer."
   negative
       ifTrue:[value := 0 - value.
               value >= 0 ifTrue: [^ self primitiveFail]]
       ifFalse:[value < 0 ifTrue:[^ self primitiveFail]].

I did not run the alien tests, so maybe I'm missing something. In any
case, background is at <http://bugs.squeak.org/view.php?id=6987>.

HTH,
Dave