[Vm-dev] What problem are we trying to solve? (was: VM Maker: VMMaker.oscog-eem.790.mcz)

[Eliot Miranda]

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/20140701/698a6830/attachment-0001.htm