<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-02 13:29 GMT+02:00 David T. Lewis <span dir="ltr">&lt;<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>&gt;</span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Eliot,<br>
<br>
Thanks for the explanation, much clearer now. I was getting lost in the<br>
email threads.<br>
<br>
Dave<br>
<br></blockquote><div><br></div><div>Yeah, Eliot did not break anything, the LargeInteger plugin was already broken.<br></div><div>Only the incorrect substitution of // by &gt;&gt; did make it work.<br></div><div>Eliot applied a minimal patch, but IMO invoking signed integer arithmetic when we do not need to is a recipe for more surprises...<br>
</div><div>This require a deeper change if we do not want to feel the ground falling from under our feet again at the least slang change.<br></div><div> <br></div><div>Nicolas<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

On Tue, Jul 01, 2014 at 08:51:26PM -0700, Eliot Miranda wrote:<br>
&gt;<br>
&gt; On Tue, Jul 1, 2014 at 8:44 PM, Eliot Miranda &lt;<a href="mailto:eliot.miranda@gmail.com">eliot.miranda@gmail.com</a>&gt;<br>
&gt; wrote:<br>
<div><div class="h5">&gt;<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt; On Tue, Jul 1, 2014 at 7:51 PM, David T. Lewis &lt;<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>&gt;<br>
&gt; &gt; wrote:<br>
&gt; &gt;<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; On Tue, Jul 01, 2014 at 05:36:54PM -0700, Eliot Miranda wrote:<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; On Tue, Jul 1, 2014 at 12:21 PM, Nicolas Cellier &lt;<br>
&gt; &gt;&gt; &gt; <a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>&gt; wrote:<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt; 2014-07-01 4:22 GMT+02:00 &lt;<a href="mailto:commits@source.squeak.org">commits@source.squeak.org</a>&gt;:<br>
&gt; &gt;&gt; &gt; &gt;&gt;<br>
&gt; &gt;&gt; &gt; &gt;&gt; Item was changed:<br>
&gt; &gt;&gt; &gt; &gt;&gt;   ----- Method: LargeIntegersPlugin&gt;&gt;cDigitSub:len:with:len:into: (in<br>
&gt; &gt;&gt; &gt; &gt;&gt; category &#39;C core&#39;) -----<br>
&gt; &gt;&gt; &gt; &gt;&gt; + cDigitSub: pByteSmall len: smallLen with: pByteLarge len: largeLen<br>
&gt; &gt;&gt; &gt; &gt;&gt; into: pByteRes<br>
&gt; &gt;&gt; &gt; &gt;&gt; +       | z |<br>
&gt; &gt;&gt; &gt; &gt;&gt; - cDigitSub: pByteSmall<br>
&gt; &gt;&gt; &gt; &gt;&gt; -               len: smallLen<br>
&gt; &gt;&gt; &gt; &gt;&gt; -               with: pByteLarge<br>
&gt; &gt;&gt; &gt; &gt;&gt; -               len: largeLen<br>
&gt; &gt;&gt; &gt; &gt;&gt; -               into: pByteRes<br>
&gt; &gt;&gt; &gt; &gt;&gt; -       | z limit |<br>
&gt; &gt;&gt; &gt; &gt;&gt;         &lt;var: #pByteSmall type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt; &gt; &gt;&gt;         &lt;var: #pByteLarge type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt; &gt; &gt;&gt;         &lt;var: #pByteRes type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt; &gt; &gt;&gt;<br>
&gt; &gt;&gt; &gt; &gt;&gt; +       z := 0. &quot;Loop invariant is -1&lt;=z&lt;=1&quot;<br>
&gt; &gt;&gt; &gt; &gt;&gt; +       0 to: smallLen - 1 do:<br>
&gt; &gt;&gt; &gt; &gt;&gt; -       z := 0.<br>
&gt; &gt;&gt; &gt; &gt;&gt; -       &quot;Loop invariant is -1&lt;=z&lt;=1&quot;<br>
&gt; &gt;&gt; &gt; &gt;&gt; -       limit := smallLen - 1.<br>
&gt; &gt;&gt; &gt; &gt;&gt; -       0 to: limit do:<br>
&gt; &gt;&gt; &gt; &gt;&gt;                 [:i |<br>
&gt; &gt;&gt; &gt; &gt;&gt;                 z := z + (pByteLarge at: i) - (pByteSmall at: i).<br>
&gt; &gt;&gt; &gt; &gt;&gt; +               pByteRes at: i put: z - (z // 256 * 256).<br>
&gt; &gt;&gt; &quot;sign-tolerant<br>
&gt; &gt;&gt; &gt; &gt;&gt; form of (z bitAnd: 255)&quot;<br>
&gt; &gt;&gt; &gt; &gt;&gt;<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt; Frankly, having z declared unsigned int and just doing  pByteRes at: i<br>
&gt; &gt;&gt; &gt; &gt; put: (z bitAnd: 16rFF) as I suggested would be way way simpler and<br>
&gt; &gt;&gt; will<br>
&gt; &gt;&gt; &gt; &gt; ALWAYS work.<br>
&gt; &gt;&gt; &gt; &gt; Why the hell invoke the complications of signed arithmetic when the<br>
&gt; &gt;&gt; &gt; &gt; content pByteRes is unsigned???<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; I&#39;m not maintaining the plugin.  But I broke it in fixing the unsigned<br>
&gt; &gt;&gt; &gt; division anomaly.  I just wanted it to work again as quickly as possibly<br>
&gt; &gt;&gt; &gt; without expending effort.  I made the minimum changes I could to keep it<br>
&gt; &gt;&gt; &gt; working.  I&#39;m much happier to have you maintain the plugin.  You have<br>
&gt; &gt;&gt; the<br>
&gt; &gt;&gt; &gt; expertise and experience.<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; Nicolas, my priority is to have Spur working.  I don&#39;t want to have to<br>
&gt; &gt;&gt; &gt; expend lots of energy changing plugins to get Spur working.  My<br>
&gt; &gt;&gt; submitting<br>
&gt; &gt;&gt; &gt; this fix is not an endorsement of any kind.  It&#39;s merely expediency.<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; After catching up with the email thread, I am confused as to what problem<br>
&gt; &gt;&gt; we<br>
&gt; &gt;&gt; are trying to solve.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; As near as I can tell, the situation is:<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - The original LargeIntegersPlugin&gt;&gt;cDigitSub:len:with:len:into: works<br>
&gt; &gt;&gt; with all<br>
&gt; &gt;&gt;    combinations of 32/64 bit VMs and images.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - Nicolas has proposed a better implementation, along with the<br>
&gt; &gt;&gt; recommendation<br>
&gt; &gt;&gt;    to use unsigned integer C arithmetic unless there is some specific<br>
&gt; &gt;&gt; good reason<br>
&gt; &gt;&gt;    to do otherwise. This seems right in principle, although the<br>
&gt; &gt;&gt; implementation in<br>
&gt; &gt;&gt;    VMMaker-nice.348 is not working for 64-bit VMs, so some issues remain<br>
&gt; &gt;&gt; to be<br>
&gt; &gt;&gt;    resolved.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - Eliot&#39;s original question began with this:<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;      &gt; I recently eliminated the optimization in Slang that replaces a<br>
&gt; &gt;&gt;      &gt; division by a power of two with a shift, because the code cast the<br>
&gt; &gt;&gt; argument<br>
&gt; &gt;&gt;      &gt; to signed, and hence broke unsigned division.  That&#39;s what used to<br>
&gt; &gt;&gt; be<br>
&gt; &gt;&gt;      &gt; controlled by the UseRightShiftForDivide class var of<br>
&gt; &gt;&gt; CCodeGenerator.<br>
&gt; &gt;&gt;      &gt;<br>
&gt; &gt;&gt;      &gt; Yesterday I found out that that optimization is the only thing<br>
&gt; &gt;&gt; that&#39;s<br>
&gt; &gt;&gt;      &gt; keeping the LargeIntegers plugin afloat.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - At that point we had a problem in the Spur/Cog VMs that led to some<br>
&gt; &gt;&gt; patching<br>
&gt; &gt;&gt;    of the code generation and so forth, along with this email thread.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; So now I am confused. Is the problem that:<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;   - The original implementation was broken?<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
</div></div>&gt; &gt; Arguably yes.  It relied on incorrect Slang translation to work,<br>
&gt; &gt; specifically the assumption that N / D where D is a power of two is<br>
&gt; &gt; equivalent to (sqInt)N &gt;&gt; S where S is log2 of D.  That&#39;s wrong on two<br>
&gt; &gt; counts:<br>
&gt; &gt;<br>
&gt; &gt;   If N is -1 then (sqInt)-1 &gt;&gt; S is -1, whereas -1 / D is zero for D &gt; 1.<br>
&gt; &gt;   If N is unsigned (sqInt)N &gt;&gt; S ~= N &gt;&gt; S if N has the top bit set.<br>
<div class="">&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt;   - There is something different in the Spur/Cog environment that exposed<br>
&gt; &gt;&gt;     problems in the original implementation?<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
</div>&gt; &gt; Yes.  I hit examples where the division optimization was generating<br>
&gt; &gt; incorrect code, e.g. in SpurMemoryManager&gt;&gt;allocateOldSpaceChunkOfBytes:<br>
&gt; &gt;<br>
&gt; &gt; initialIndex := chunkBytes / self allocationUnit.<br>
&gt; &gt;<br>
&gt; &gt; even though chunkBytes is unsigned, Slang was generating (sqInt)chunkBytes<br>
&gt; &gt; &gt;&gt; 3, which generates garbage when chunkBytes &gt;= 2^31.  I decided to rip<br>
&gt; &gt; out the optimization (it is incorrect in the -1 / D case) rather than hack<br>
&gt; &gt; these.  That in turn surfaced the bug in the LargeIntegers plugin.<br>
<div class="">&gt; &gt;<br>
&gt; &gt;   - The original implementation worked, but the change to CCodeGenerator<br>
&gt; &gt;&gt; with<br>
&gt; &gt;&gt;     regard to its use of UseRightShiftForDivide resulted in a problem?<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
</div>&gt; &gt; Yes.<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt;&gt;   - Something else?<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
&gt; &gt; No.<br>
<div class="">&gt; &gt;<br>
&gt; &gt; I&#39;m pretty sure I&#39;m missing something here.<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
</div>&gt; &gt; You don&#39;t appear to be.<br>
&gt; &gt;<br>
&gt; &gt; and underlying this is that philosophically I believe that a) having Slang<br>
&gt; generate correct code and b) having Slang maintain as close a<br>
&gt; correspondence between Smalltalk and the resulting C is to be preferred 1)<br>
&gt; at the risk of breaking plugins (which then have to be fixed), and 2)<br>
&gt; papering over the cracks by using cCode: [] inSmalltalk: [] everywhere.<br>
&gt;<br>
&gt; ANother way of saying this is that I believe Slang should be approachable<br>
&gt; by the newbie who enters with rational expectations, not something only old<br>
&gt; salts who know and work-around the many pitfalls.  I got horribly burned by<br>
&gt; Slang often in the first few years of working on Cog.  I likened it to<br>
&gt; being hit on the head with a long stick by one&#39;s sensei, except that no<br>
&gt; enlightenment would ever ensue.  One&#39;s head would simply hurt.  I&#39;ve tried<br>
&gt; to make it better, e.g. by adding some simple type inference so that it<br>
&gt; does the right thing with unsigned and long long types.  Alas, when one<br>
&gt; does this old workarounds, mistakes, or bugs may break.<br>
<span class="HOEnZb"><font color="#888888">&gt; --<br>
&gt; best,<br>
&gt; Eliot<br>
<br>
</font></span></blockquote></div><br></div></div>