<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 1, 2014 at 7:51 PM, David T. Lewis <span dir="ltr">&lt;<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
On Tue, Jul 01, 2014 at 05:36:54PM -0700, Eliot Miranda wrote:<br>
&gt;<br>
&gt; On Tue, Jul 1, 2014 at 12:21 PM, Nicolas Cellier &lt;<br>
&gt; <a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>&gt; wrote:<br>
&gt;<br>
&gt; &gt;<br>
&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;<br>
&gt; &gt;&gt; Item was changed:<br>
&gt; &gt;&gt;   ----- Method: LargeIntegersPlugin&gt;&gt;cDigitSub:len:with:len:into: (in<br>
&gt; &gt;&gt; category &#39;C core&#39;) -----<br>
&gt; &gt;&gt; + cDigitSub: pByteSmall len: smallLen with: pByteLarge len: largeLen<br>
&gt; &gt;&gt; into: pByteRes<br>
&gt; &gt;&gt; +       | z |<br>
&gt; &gt;&gt; - cDigitSub: pByteSmall<br>
&gt; &gt;&gt; -               len: smallLen<br>
&gt; &gt;&gt; -               with: pByteLarge<br>
&gt; &gt;&gt; -               len: largeLen<br>
&gt; &gt;&gt; -               into: pByteRes<br>
&gt; &gt;&gt; -       | z limit |<br>
&gt; &gt;&gt;         &lt;var: #pByteSmall type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt;         &lt;var: #pByteLarge type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt;         &lt;var: #pByteRes type: &#39;unsigned char * &#39;&gt;<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; +       z := 0. &quot;Loop invariant is -1&lt;=z&lt;=1&quot;<br>
&gt; &gt;&gt; +       0 to: smallLen - 1 do:<br>
&gt; &gt;&gt; -       z := 0.<br>
&gt; &gt;&gt; -       &quot;Loop invariant is -1&lt;=z&lt;=1&quot;<br>
&gt; &gt;&gt; -       limit := smallLen - 1.<br>
&gt; &gt;&gt; -       0 to: limit do:<br>
&gt; &gt;&gt;                 [:i |<br>
&gt; &gt;&gt;                 z := z + (pByteLarge at: i) - (pByteSmall at: i).<br>
&gt; &gt;&gt; +               pByteRes at: i put: z - (z // 256 * 256). &quot;sign-tolerant<br>
&gt; &gt;&gt; form of (z bitAnd: 255)&quot;<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
&gt; &gt; Frankly, having z declared unsigned int and just doing  pByteRes at: i<br>
&gt; &gt; put: (z bitAnd: 16rFF) as I suggested would be way way simpler and will<br>
&gt; &gt; ALWAYS work.<br>
&gt; &gt; Why the hell invoke the complications of signed arithmetic when the<br>
&gt; &gt; content pByteRes is unsigned???<br>
&gt; &gt;<br>
&gt;<br>
&gt; I&#39;m not maintaining the plugin.  But I broke it in fixing the unsigned<br>
&gt; division anomaly.  I just wanted it to work again as quickly as possibly<br>
&gt; without expending effort.  I made the minimum changes I could to keep it<br>
&gt; working.  I&#39;m much happier to have you maintain the plugin.  You have the<br>
&gt; expertise and experience.<br>
&gt;<br>
&gt; Nicolas, my priority is to have Spur working.  I don&#39;t want to have to<br>
&gt; expend lots of energy changing plugins to get Spur working.  My submitting<br>
&gt; this fix is not an endorsement of any kind.  It&#39;s merely expediency.<br>
&gt;<br>
<br>
After catching up with the email thread, I am confused as to what problem we<br>
are trying to solve.<br>
<br>
As near as I can tell, the situation is:<br>
<br>
 - The original LargeIntegersPlugin&gt;&gt;cDigitSub:len:with:len:into: works with all<br>
   combinations of 32/64 bit VMs and images.<br>
<br>
 - Nicolas has proposed a better implementation, along with the recommendation<br>
   to use unsigned integer C arithmetic unless there is some specific good reason<br>
   to do otherwise. This seems right in principle, although the implementation in<br>
   VMMaker-nice.348 is not working for 64-bit VMs, so some issues remain to be<br>
   resolved.<br>
<br>
 - Eliot&#39;s original question began with this:<br>
<br>
     &gt; I recently eliminated the optimization in Slang that replaces a<br>
     &gt; division by a power of two with a shift, because the code cast the argument<br>
     &gt; to signed, and hence broke unsigned division.  That&#39;s what used to be<br>
     &gt; controlled by the UseRightShiftForDivide class var of CCodeGenerator.<br>
     &gt;<br>
     &gt; Yesterday I found out that that optimization is the only thing that&#39;s<br>
     &gt; keeping the LargeIntegers plugin afloat.<br>
<br>
 - At that point we had a problem in the Spur/Cog VMs that led to some patching<br>
   of the code generation and so forth, along with this email thread.<br>
<br>
So now I am confused. Is the problem that:<br>
<br>
  - The original implementation was broken?<br></blockquote><div><br></div><div>Arguably yes.  It relied on incorrect Slang translation to work, specifically the assumption that N / D where D is a power of two is equivalent to (sqInt)N &gt;&gt; S where S is log2 of D.  That&#39;s wrong on two counts:</div>
<div><br></div><div>  If N is -1 then (sqInt)-1 &gt;&gt; S is -1, whereas -1 / D is zero for D &gt; 1.</div><div>  If N is unsigned (sqInt)N &gt;&gt; S ~= N &gt;&gt; S if N has the top bit set.</div><div><br></div><div><br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">  - There is something different in the Spur/Cog environment that exposed<br>

    problems in the original implementation?<br></blockquote><div><br></div><div>Yes.  I hit examples where the division optimization was generating incorrect code, e.g. in SpurMemoryManager&gt;&gt;allocateOldSpaceChunkOfBytes:</div>
<div><br></div><div><span class="" style="white-space:pre">        </span>initialIndex := chunkBytes / self allocationUnit.<br></div><div><br></div><div>even though chunkBytes is unsigned, Slang was generating (sqInt)chunkBytes &gt;&gt; 3, which generates garbage when chunkBytes &gt;= 2^31.  I decided to rip out the optimization (it is incorrect in the -1 / D case) rather than hack these.  That in turn surfaced the bug in the LargeIntegers plugin.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">  - The original implementation worked, but the change to CCodeGenerator with<br>

    regard to its use of UseRightShiftForDivide resulted in a problem?<br></blockquote><div><br></div><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

  - Something else?<br></blockquote><div><br></div><div>No.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I&#39;m pretty sure I&#39;m missing something here.<br></blockquote><div><br></div><div>You don&#39;t appear to be. </div></div>-- <br>best,<div>Eliot</div>
</div></div>