<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 2, 2014 at 7:54 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
On Wed, Jul 2, 2014 at 4:53 AM, Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.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><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"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>></span>:<div class=""><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>
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><div>Yeah, Eliot did not break anything, the LargeInteger plugin was already broken.<br></div><div>Only the incorrect substitution of // by >> 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></div></div></blockquote><div><br></div><div>Alas I think it's still very broken. I see a lot of failures and errors in IntegerTest, LargePositiveTest & LargeNegativeTest. I'm going to try your 32-bit plugin and if that works, build and release new VMs asap. But I need to go through the plugins that changed in 3021 (generating without the broken division-by-power-of-two optimization) with a fine toothcomb:</div>
<div><br></div><div><div>B2DPlugin.c</div><div>B3DAcceleratorPlugin.c</div><div>BitBltPlugin.c</div><div>BochsIA32Plugin.c</div><div>FFTPlugin.c</div><div>FilePlugin.c</div><div>IA32ABI.c</div><div>JPEGReaderPlugin.c</div>
<div>Klatt.c</div><div>MacMenubarPlugin.c</div><div>MiscPrimitivePlugin.c</div><div>QuicktimePlugin.c</div><div>RePlugin.c</div><div>ScratchPlugin.c</div><div>SoundGenerationPlugin.c</div><div>ARM32FFIPlugin.c</div><div>
IA32FFIPlugin.c</div>
<div>ZipPlugin.c</div></div><div><br></div><div>This is work I don't want to do. I either revert back to the broken division optimization, and add cCode:inSmalltalk: hacks in Spur, or have to fix these *%^& plugins. Damned if I do, damned if I don't. See what I mean about being hit on the head.</div>
<div><br></div></div></div></div></blockquote><div><br></div><div>Not so bad. The list is smaller than I thought:</div><div><br></div><div><div><div>B2DPlugin.c</div><div>BitBltPlugin.c</div><div>FFTPlugin.c</div><div>JPEGReaderPlugin.c</div>
<div>Klatt.c</div><div>LargeIntegers.c</div><div>MiscPrimitivePlugin.c</div><div>RePlugin.c</div><div>ScratchPlugin.c</div><div>SoundGenerationPlugin.c</div><div>ZipPlugin.c</div></div></div><div><br></div><div><br></div>
<div>The others simply changed their declaration of positive32BitValueOf</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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></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">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div></div><div> <br></div><div>Nicolas<br></div><div><div class="h5"><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">
On Tue, Jul 01, 2014 at 08:51:26PM -0700, Eliot Miranda wrote:<br>
><br>
> On Tue, Jul 1, 2014 at 8:44 PM, Eliot Miranda <<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>><br>
> wrote:<br>
<div><div>><br>
> ><br>
> ><br>
> ><br>
> > On Tue, Jul 1, 2014 at 7:51 PM, David T. Lewis <<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>><br>
> > wrote:<br>
> ><br>
> >><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).<br>
> >> "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<br>
> >> 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<br>
> >> 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<br>
> >> 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<br>
> >> 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<br>
> >> with all<br>
> >> combinations of 32/64 bit VMs and images.<br>
> >><br>
> >> - Nicolas has proposed a better implementation, along with the<br>
> >> recommendation<br>
> >> to use unsigned integer C arithmetic unless there is some specific<br>
> >> good reason<br>
> >> to do otherwise. This seems right in principle, although the<br>
> >> implementation in<br>
> >> VMMaker-nice.348 is not working for 64-bit VMs, so some issues remain<br>
> >> 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<br>
> >> argument<br>
> >> > to signed, and hence broke unsigned division. That's what used to<br>
> >> be<br>
> >> > controlled by the UseRightShiftForDivide class var of<br>
> >> CCodeGenerator.<br>
> >> ><br>
> >> > Yesterday I found out that that optimization is the only thing<br>
> >> 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<br>
> >> 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>
> >><br>
> ><br>
</div></div>> > Arguably yes. It relied on incorrect Slang translation to work,<br>
> > specifically the assumption that N / D where D is a power of two is<br>
> > equivalent to (sqInt)N >> S where S is log2 of D. That's wrong on two<br>
> > counts:<br>
> ><br>
> > If N is -1 then (sqInt)-1 >> S is -1, whereas -1 / D is zero for D > 1.<br>
> > If N is unsigned (sqInt)N >> S ~= N >> S if N has the top bit set.<br>
<div>> ><br>
> ><br>
> > - There is something different in the Spur/Cog environment that exposed<br>
> >> problems in the original implementation?<br>
> >><br>
> ><br>
</div>> > Yes. I hit examples where the division optimization was generating<br>
> > incorrect code, e.g. in SpurMemoryManager>>allocateOldSpaceChunkOfBytes:<br>
> ><br>
> > initialIndex := chunkBytes / self allocationUnit.<br>
> ><br>
> > even though chunkBytes is unsigned, Slang was generating (sqInt)chunkBytes<br>
> > >> 3, which generates garbage when chunkBytes >= 2^31. I decided to rip<br>
> > out the optimization (it is incorrect in the -1 / D case) rather than hack<br>
> > these. That in turn surfaced the bug in the LargeIntegers plugin.<br>
<div>> ><br>
> > - The original implementation worked, but the change to CCodeGenerator<br>
> >> with<br>
> >> regard to its use of UseRightShiftForDivide resulted in a problem?<br>
> >><br>
> ><br>
</div>> > Yes.<br>
> ><br>
> ><br>
> >> - Something else?<br>
> >><br>
> ><br>
> > No.<br>
<div>> ><br>
> > I'm pretty sure I'm missing something here.<br>
> >><br>
> ><br>
</div>> > You don't appear to be.<br>
> ><br>
> > and underlying this is that philosophically I believe that a) having Slang<br>
> generate correct code and b) having Slang maintain as close a<br>
> correspondence between Smalltalk and the resulting C is to be preferred 1)<br>
> at the risk of breaking plugins (which then have to be fixed), and 2)<br>
> papering over the cracks by using cCode: [] inSmalltalk: [] everywhere.<br>
><br>
> ANother way of saying this is that I believe Slang should be approachable<br>
> by the newbie who enters with rational expectations, not something only old<br>
> salts who know and work-around the many pitfalls. I got horribly burned by<br>
> Slang often in the first few years of working on Cog. I likened it to<br>
> being hit on the head with a long stick by one's sensei, except that no<br>
> enlightenment would ever ensue. One's head would simply hurt. I've tried<br>
> to make it better, e.g. by adding some simple type inference so that it<br>
> does the right thing with unsigned and long long types. Alas, when one<br>
> does this old workarounds, mistakes, or bugs may break.<br>
<span><font color="#888888">> --<br>
> best,<br>
> Eliot<br>
<br>
</font></span></blockquote></div></div></div><br></div></div>
<br></blockquote></div><span class=""><font color="#888888"><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</font></span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</div></div>