Hi Stefan,<div><br></div><div>    so the VMMaker source is probably easier to read.  First, thanks for the optimization; clearly</div><div><br></div><div><div><span class="Apple-tab-span" style="white-space:pre">                                </span>(arg = 0 or: [(result // arg) = rcvr and: [self isIntegerValue: result]])</div>
</div><div><br></div><div><br></div><div>is better than the original</div><div><br></div><div><div><span class="Apple-tab-span" style="white-space:pre">                                </span>((arg = 0 or: [(result // arg) = rcvr]) and: [self isIntegerValue: result])</div>
<div><br></div><div><br></div>I&#39;d suspect the definition of isIntegerValue:, which is:</div><div><br></div><div>ObjectMemory methods for interpreter access</div><div><div>isIntegerValue: intValue</div><div><span class="Apple-tab-span" style="white-space:pre">        </span>&quot;Return true if the given value can be represented as a Smalltalk integer value.&quot;</div>
<div><span class="Apple-tab-span" style="white-space:pre">        </span>&quot;Use a shift and XOR to set the sign bit if and only if the top two bits of the given value are the same, then test the sign bit. Note that the top two bits are equal for exactly those integers in the range that can be represented in 31-bits or 63-bits.&quot;</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space:pre">        </span>^ (intValue bitXor: (intValue &lt;&lt; 1)) &gt;= 0</div><div><br></div><div>The comment makes sense but there is perhaps nothing to guarantee that a C compiler must consider the type of signed ^ signed as signed, in which case the compiler would assume it is always &gt;= 0, and the overflow detection would fail.</div>
<div><br></div><div>If you change the method to</div><div><br></div><div><div>ObjectMemory methods for interpreter access</div><div><div>isIntegerValue: intValue</div><div><span class="Apple-tab-span" style="white-space: pre; ">        </span>&quot;Return true if the given value can be represented as a Smalltalk integer value.&quot;</div>
<div><span class="Apple-tab-span" style="white-space: pre; ">        </span>&quot;Use a shift and XOR to set the sign bit if and only if the top two bits of the given value are the same, then test the sign bit. Note that the top two bits are equal for exactly those integers in the range that can be represented in 31-bits or 63-bits.&quot;</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space: pre; ">        </span>^(intValue bitXor: (intValue &lt;&lt; 1)) asInteger &gt;= 0</div></div></div><div><br></div><div>which should force a cast to sqInt, does that fix things?</div>
<div><br></div><div>i.e. it should produce</div><div><br></div><div><div>      if (((arg == 0)</div><div>       || ((result / arg) == rcvr))</div><div>        &amp;&amp; ((sqInt)((result ^ (result &lt;&lt; 1))) &gt;= 0)) {</div>
</div><div><br></div><div class="gmail_quote">On Mon, Oct 3, 2011 at 5:46 PM, Stefan Marr <span dir="ltr">&lt;<a href="mailto:squeak@stefan-marr.de">squeak@stefan-marr.de</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Dear all:<br>
<br>
With the RoarVM, I was hitting a problem with Clang 3.0 (tags/Apple/clang-211.9).<br>
<br>
It turned out that Clang does not like the current implementation of bytecodePrimMultiply.<br>
<br>
>From what I can tell, it should be identical with what I found in the SqueakVM sources.<br>
<br>
RoarVM src: <a href="https://github.com/smarr/RoarVM/blob/master/vm/src/interpreter/interpreter_bytecodes.cpp#L316" target="_blank">https://github.com/smarr/RoarVM/blob/master/vm/src/interpreter/interpreter_bytecodes.cpp#L316</a><br>

<br>
    oop_int_t ri = rcvr.integerValue();<br>
    oop_int_t ai = arg.integerValue();<br>
    oop_int_t r  = ri * ai;<br>
    if (ai == 0  ||  (r / ai  ==  ri    &amp;&amp;    Oop::isIntegerValue(r))) {<br>
      internalPopThenPush(2, Oop::from_int(r));<br>
      fetchNextBytecode();<br>
      return;<br>
    }<br>
<br>
Squeak src:<br>
<a href="http://squeakvm.org/cgi-bin/viewcvs.cgi/branches/Cog/src/vm/cointerp.c?rev=2497&amp;view=auto" target="_blank">http://squeakvm.org/cgi-bin/viewcvs.cgi/branches/Cog/src/vm/cointerp.c?rev=2497&amp;view=auto</a><br>

<br>
        rcvr = (rcvr &gt;&gt; 1);<br>
        arg = (arg &gt;&gt; 1);<br>
        result = rcvr * arg;<br>
        if (((arg == 0)<br>
        || ((result / arg) == rcvr))<br>
         &amp;&amp; ((result ^ (result &lt;&lt; 1)) &gt;= 0)) {<br>
                /* begin internalPop:thenPush: */<br>
                longAtPointerput((localSP += (2 - 1) * BytesPerWord), ((result &lt;&lt; 1) | 1));<br>
                /* begin fetchNextBytecode */<br>
                currentBytecode = byteAtPointer(++localIP);<br>
                goto l69;<br>
        }<br>
<br>
<br>
The problematic example is rcvr: 40453, arg: 864001.<br>
<br>
In that case, the current implementation does not detect the overflow correctly when Clang is used with &gt;= O1 optimization.<br>
<br>
The big question for me now is, whether we got here a Clang compiler bug, or unspecified C behavior.<br>
<br>
<br>
My quick fix is:<br>
<br>
    oop_int_t ri = rcvr.integerValue();<br>
    oop_int_t ai = arg.integerValue();<br>
    long long result_with_overflow = (long long)ri * ai;<br>
    if (ai == 0  ||  ((result_with_overflow &gt;= -1073741824) &amp;&amp; (result_with_overflow &lt;= 1073741823)))<br>
    {<br>
      internalPopThenPush(2, Oop::from_int(result_with_overflow));<br>
      fetchNextBytecode();<br>
      return;<br>
    }<br>
<br>
This works obviously, but I assume that the original code is a well considered speed optimizations.<br>
<br>
Any thoughts?<br>
<br>
Thanks<br>
Stefan<br>
<font color="#888888"><br>
--<br>
Stefan Marr<br>
Software Languages Lab<br>
Vrije Universiteit Brussel<br>
Pleinlaan 2 / B-1050 Brussels / Belgium<br>
<a href="http://soft.vub.ac.be/~smarr" target="_blank">http://soft.vub.ac.be/~smarr</a><br>
Phone: <a href="tel:%2B32%202%20629%202974" value="+3226292974">+32 2 629 2974</a><br>
Fax:   <a href="tel:%2B32%202%20629%203525" value="+3226293525">+32 2 629 3525</a><br>
<br>
</font></blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div><br>
</div>