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

[Eliot Miranda]

Hi David,

On Mon, Jul 25, 2011 at 6:27 AM, David T. Lewis <lewis at mail.msen.com> wrote:

>
> 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.


Alas, no, it still exists.  The expression 0 - value is still undefined for
overflow (i.e. undefined for max neg int) and hence, depending on compiler,
the following statement value >= 0 may be assumed to be always true.  This
is the case for the intel compiler that we used at Qwaq/Teleplace, and is
the reason I made the change in the first place.  So yes, one does need the
shift expression to be able to rely on different compilers generating
correct code in this edge case.




> 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
>
>


-- 
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20110725/e165702d/attachment.htm