<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 1, 2014 at 8:44 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">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" target="_blank">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" target="_blank">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></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 class=""><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><div>Yes. I hit examples where the division optimization was generating incorrect code, e.g. in SpurMemoryManager>>allocateOldSpaceChunkOfBytes:</div>
<div><br></div><div><span style="white-space:pre-wrap"> </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 class="">
<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><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 class=""><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><div>You don't appear to be. </div><div><br></div></div></div></div></blockquote><div>and underlying this is that philosophically I believe that a) having Slang generate correct code and b) having Slang maintain as close a correspondence between Smalltalk and the resulting C is to be preferred 1) at the risk of breaking plugins (which then have to be fixed), and 2) papering over the cracks by using cCode: [] inSmalltalk: [] everywhere.</div>
<div><br></div><div>ANother way of saying this is that I believe Slang should be approachable by the newbie who enters with rational expectations, not something only old salts who know and work-around the many pitfalls. I got horribly burned by Slang often in the first few years of working on Cog. I likened it to being hit on the head with a long stick by one's sensei, except that no enlightenment would ever ensue. One's head would simply hurt. I've tried to make it better, e.g. by adding some simple type inference so that it does the right thing with unsigned and long long types. Alas, when one does this old workarounds, mistakes, or bugs may break.</div>
</div>-- <br>best,<div>Eliot</div>
</div></div>