<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"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>></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>
><br>
> On Tue, Jul 1, 2014 at 12:21 PM, Nicolas Cellier <<br>
> <a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>> wrote:<br>
><br>
> ><br>
> > 2014-07-01 4:22 GMT+02:00 <<a href="mailto:commits@source.squeak.org">commits@source.squeak.org</a>>:<br>
> >><br>
> >> Item was changed:<br>
> >> ----- Method: LargeIntegersPlugin>>cDigitSub:len:with:len:into: (in<br>
> >> category 'C core') -----<br>
> >> + cDigitSub: pByteSmall len: smallLen with: pByteLarge len: largeLen<br>
> >> into: pByteRes<br>
> >> + | z |<br>
> >> - cDigitSub: pByteSmall<br>
> >> - len: smallLen<br>
> >> - with: pByteLarge<br>
> >> - len: largeLen<br>
> >> - into: pByteRes<br>
> >> - | z limit |<br>
> >> <var: #pByteSmall type: 'unsigned char * '><br>
> >> <var: #pByteLarge type: 'unsigned char * '><br>
> >> <var: #pByteRes type: 'unsigned char * '><br>
> >><br>
> >> + z := 0. "Loop invariant is -1<=z<=1"<br>
> >> + 0 to: smallLen - 1 do:<br>
> >> - z := 0.<br>
> >> - "Loop invariant is -1<=z<=1"<br>
> >> - limit := smallLen - 1.<br>
> >> - 0 to: limit do:<br>
> >> [:i |<br>
> >> z := z + (pByteLarge at: i) - (pByteSmall at: i).<br>
> >> + pByteRes at: i put: z - (z // 256 * 256). "sign-tolerant<br>
> >> form of (z bitAnd: 255)"<br>
> >><br>
> ><br>
> > Frankly, having z declared unsigned int and just doing pByteRes at: i<br>
> > put: (z bitAnd: 16rFF) as I suggested would be way way simpler and will<br>
> > ALWAYS work.<br>
> > Why the hell invoke the complications of signed arithmetic when the<br>
> > content pByteRes is unsigned???<br>
> ><br>
><br>
> I'm not maintaining the plugin. But I broke it in fixing the unsigned<br>
> division anomaly. I just wanted it to work again as quickly as possibly<br>
> without expending effort. I made the minimum changes I could to keep it<br>
> working. I'm much happier to have you maintain the plugin. You have the<br>
> expertise and experience.<br>
><br>
> Nicolas, my priority is to have Spur working. I don't want to have to<br>
> expend lots of energy changing plugins to get Spur working. My submitting<br>
> this fix is not an endorsement of any kind. It's merely expediency.<br>
><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>>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's original question began with this:<br>
<br>
> I recently eliminated the optimization in Slang that replaces a<br>
> division by a power of two with a shift, because the code cast the argument<br>
> to signed, and hence broke unsigned division. That's what used to be<br>
> controlled by the UseRightShiftForDivide class var of CCodeGenerator.<br>
><br>
> Yesterday I found out that that optimization is the only thing that's<br>
> 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 >> S where S is log2 of D. That's wrong on two counts:</div>
<div><br></div><div> If N is -1 then (sqInt)-1 >> S is -1, whereas -1 / D is zero for D > 1.</div><div> If N is unsigned (sqInt)N >> S ~= N >> 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>>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 >> 3, which generates garbage when chunkBytes >= 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'm pretty sure I'm missing something here.<br></blockquote><div><br></div><div>You don't appear to be. </div></div>-- <br>best,<div>Eliot</div>
</div></div>