[Vm-dev] What problem are we trying to solve? (was: VM Maker:
VMMaker.oscog-eem.790.mcz)
Nicolas Cellier
nicolas.cellier.aka.nice at gmail.com
Wed Jul 2 11:53:12 UTC 2014
2014-07-02 13:29 GMT+02:00 David T. Lewis <lewis at mail.msen.com>:
>
> Eliot,
>
> Thanks for the explanation, much clearer now. I was getting lost in the
> email threads.
>
> Dave
>
>
Yeah, Eliot did not break anything, the LargeInteger plugin was already
broken.
Only the incorrect substitution of // by >> did make it work.
Eliot applied a minimal patch, but IMO invoking signed integer arithmetic
when we do not need to is a recipe for more surprises...
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.
Nicolas
On Tue, Jul 01, 2014 at 08:51:26PM -0700, Eliot Miranda wrote:
> >
> > On Tue, Jul 1, 2014 at 8:44 PM, Eliot Miranda <eliot.miranda at gmail.com>
> > wrote:
> >
> > >
> > >
> > >
> > > On Tue, Jul 1, 2014 at 7:51 PM, David T. Lewis <lewis at mail.msen.com>
> > > wrote:
> > >
> > >>
> > >> On Tue, Jul 01, 2014 at 05:36:54PM -0700, Eliot Miranda wrote:
> > >> >
> > >> > On Tue, Jul 1, 2014 at 12:21 PM, Nicolas Cellier <
> > >> > nicolas.cellier.aka.nice at gmail.com> wrote:
> > >> >
> > >> > >
> > >> > > 2014-07-01 4:22 GMT+02:00 <commits at source.squeak.org>:
> > >> > >>
> > >> > >> Item was changed:
> > >> > >> ----- Method:
> LargeIntegersPlugin>>cDigitSub:len:with:len:into: (in
> > >> > >> category 'C core') -----
> > >> > >> + cDigitSub: pByteSmall len: smallLen with: pByteLarge len:
> largeLen
> > >> > >> into: pByteRes
> > >> > >> + | z |
> > >> > >> - cDigitSub: pByteSmall
> > >> > >> - len: smallLen
> > >> > >> - with: pByteLarge
> > >> > >> - len: largeLen
> > >> > >> - into: pByteRes
> > >> > >> - | z limit |
> > >> > >> <var: #pByteSmall type: 'unsigned char * '>
> > >> > >> <var: #pByteLarge type: 'unsigned char * '>
> > >> > >> <var: #pByteRes type: 'unsigned char * '>
> > >> > >>
> > >> > >> + z := 0. "Loop invariant is -1<=z<=1"
> > >> > >> + 0 to: smallLen - 1 do:
> > >> > >> - z := 0.
> > >> > >> - "Loop invariant is -1<=z<=1"
> > >> > >> - limit := smallLen - 1.
> > >> > >> - 0 to: limit do:
> > >> > >> [:i |
> > >> > >> z := z + (pByteLarge at: i) - (pByteSmall at: i).
> > >> > >> + pByteRes at: i put: z - (z // 256 * 256).
> > >> "sign-tolerant
> > >> > >> form of (z bitAnd: 255)"
> > >> > >>
> > >> > >
> > >> > > Frankly, having z declared unsigned int and just doing pByteRes
> at: i
> > >> > > put: (z bitAnd: 16rFF) as I suggested would be way way simpler and
> > >> will
> > >> > > ALWAYS work.
> > >> > > Why the hell invoke the complications of signed arithmetic when
> the
> > >> > > content pByteRes is unsigned???
> > >> > >
> > >> >
> > >> > I'm not maintaining the plugin. But I broke it in fixing the
> unsigned
> > >> > division anomaly. I just wanted it to work again as quickly as
> possibly
> > >> > without expending effort. I made the minimum changes I could to
> keep it
> > >> > working. I'm much happier to have you maintain the plugin. You
> have
> > >> the
> > >> > expertise and experience.
> > >> >
> > >> > Nicolas, my priority is to have Spur working. I don't want to have
> to
> > >> > expend lots of energy changing plugins to get Spur working. My
> > >> submitting
> > >> > this fix is not an endorsement of any kind. It's merely expediency.
> > >> >
> > >>
> > >> After catching up with the email thread, I am confused as to what
> problem
> > >> we
> > >> are trying to solve.
> > >>
> > >> As near as I can tell, the situation is:
> > >>
> > >> - The original LargeIntegersPlugin>>cDigitSub:len:with:len:into:
> works
> > >> with all
> > >> combinations of 32/64 bit VMs and images.
> > >>
> > >> - Nicolas has proposed a better implementation, along with the
> > >> recommendation
> > >> to use unsigned integer C arithmetic unless there is some specific
> > >> good reason
> > >> to do otherwise. This seems right in principle, although the
> > >> implementation in
> > >> VMMaker-nice.348 is not working for 64-bit VMs, so some issues
> remain
> > >> to be
> > >> resolved.
> > >>
> > >> - Eliot's original question began with this:
> > >>
> > >> > I recently eliminated the optimization in Slang that replaces a
> > >> > division by a power of two with a shift, because the code cast
> the
> > >> argument
> > >> > to signed, and hence broke unsigned division. That's what
> used to
> > >> be
> > >> > controlled by the UseRightShiftForDivide class var of
> > >> CCodeGenerator.
> > >> >
> > >> > Yesterday I found out that that optimization is the only thing
> > >> that's
> > >> > keeping the LargeIntegers plugin afloat.
> > >>
> > >> - At that point we had a problem in the Spur/Cog VMs that led to some
> > >> patching
> > >> of the code generation and so forth, along with this email thread.
> > >>
> > >> So now I am confused. Is the problem that:
> > >>
> > >> - The original implementation was broken?
> > >>
> > >
> > > 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:
> > >
> > > If N is -1 then (sqInt)-1 >> S is -1, whereas -1 / D is zero for D >
> 1.
> > > If N is unsigned (sqInt)N >> S ~= N >> S if N has the top bit set.
> > >
> > >
> > > - There is something different in the Spur/Cog environment that
> exposed
> > >> problems in the original implementation?
> > >>
> > >
> > > Yes. I hit examples where the division optimization was generating
> > > incorrect code, e.g. in
> SpurMemoryManager>>allocateOldSpaceChunkOfBytes:
> > >
> > > initialIndex := chunkBytes / self allocationUnit.
> > >
> > > 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.
> > >
> > > - The original implementation worked, but the change to
> CCodeGenerator
> > >> with
> > >> regard to its use of UseRightShiftForDivide resulted in a problem?
> > >>
> > >
> > > Yes.
> > >
> > >
> > >> - Something else?
> > >>
> > >
> > > No.
> > >
> > > I'm pretty sure I'm missing something here.
> > >>
> > >
> > > You don't appear to be.
> > >
> > > 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.
> >
> > 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.
> > --
> > best,
> > Eliot
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140702/19fa28e2/attachment.htm
More information about the Vm-dev
mailing list