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

[David T. Lewis]

On Thu, Jul 21, 2011 at 03:24:08PM -0700, Eliot Miranda wrote:
>  
> On Thu, Jul 21, 2011 at 3:17 PM, Stefan Marr <squeak at stefan-marr.de> wrote:
> 
> >
> > Hi Eliot:
> >
> > On 21/07/11 19:35, Eliot Miranda wrote:
> >
> >
> >> On Thu, Jul 21, 2011 at 4:30 AM, David T. Lewis <lewis at mail.msen.com<mailto:
> >> lewis at mail.msen.com>> 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