[Vm-dev] VM Maker: VMMaker.oscog-nice.2723.mcz

Eliot Miranda eliot.miranda at gmail.com
Tue Mar 10 21:25:32 UTC 2020


Hi Nicolas,

On Tue, Mar 10, 2020 at 2:15 PM Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com> wrote:

>
> Hi Levente,
> for some reason, early VM writers were looking for a Logical Right Shift
> (not propagating the sign bit).
> So that's how bitShift: (then >>) were historically translated - no matter
> how weird or contradictory to VM simulation it can be.
> I guess that it is of greatest interest for generating BitBlt operations
> (Simulation works ok because WordArray are like unsigned).
>

That's right.  For graphics one wants logical right shifts.  Also some
times convenient for bit field extractions in object headers, etc.  If one
is extracting a field that includes the MSB (sign bit) then a logical shift
yields the result without needing to mask.


>
> Eliot had to later introduce >>> for Arithmetic Right Shift and
> signedBitShift: too, because we sometimes need those operations too.
> That makes one more surprise because we now have the exact opposite of
> Java semantics for >> and >>> !
>

:-)


> We have another similar surprising behavior with translation of // and \\
> which are translated into C operations equivalent  to quo: and rem:
>
> https://smallissimo.blogspot.com/2015/03/is-bitshift-equivalent-to-division-in.html?m=0
>
> These hackish choices are unfortunate, but very difficult to change now
> without hiccups...
>

+1 ish.  The important thing is to get the VM to work and spending lots of
time making sure the C is beautiful (rather than just debuggable) is taking
time away from more productive tasks.  We really are treating C as an
assembler and for that the recent introduction of undefined behavior for
may operations that were perfectly well defined on 32-bit 2's compliment
machines was a PITA ;-)


> Le mar. 10 mars 2020 à 21:55, Levente Uzonyi <leves at caesar.elte.hu> a
> écrit :
>
>>
>> Hi Nicolas,
>>
>> Thanks for the fix. It works.
>> I wonder why is it necessary to cast to usqInt. Is it just to ensure
>> unsignedness?
>>
>>
>> Levente
>>
>> On Mon, 9 Mar 2020, commits at source.squeak.org wrote:
>>
>> >
>> > Nicolas Cellier uploaded a new version of VMMaker to project VM Maker:
>> > http://source.squeak.org/VMMaker/VMMaker.oscog-nice.2723.mcz
>> >
>> > ==================== Summary ====================
>> >
>> > Name: VMMaker.oscog-nice.2723
>> > Author: nice
>> > Time: 10 March 2020, 12:26:31.04183 am
>> > UUID: c1319382-406c-43a7-9f55-2b48c4007d80
>> > Ancestors: VMMaker.oscog-eem.2722
>> >
>> > Fix the right shift: dont convert to usqInt a type longer than usqInt
>> >
>> > =============== Diff against VMMaker.oscog-eem.2722 ===============
>> >
>> > Item was changed:
>> >  ----- Method: CCodeGenerator>>generateBitShift:on:indent: (in category
>> 'C translation') -----
>> >  generateBitShift: msgNode on: aStream indent: level
>> >       "Generate the C code for this message onto the given stream."
>> >
>> > +     | arg shift rightShift |
>> > +     (self isConstantNode: (arg := msgNode args first) valueInto:
>> [:shiftValue| shift := shiftValue])
>> > -     | arg rcvr shift |
>> > -     arg := msgNode args first.
>> > -     rcvr := msgNode receiver.
>> > -     (self isConstantNode: arg valueInto: [:shiftValue| shift :=
>> shiftValue])
>> >               ifTrue: "bit shift amount is a constant"
>> > +                     [aStream nextPut: $(.
>> > -                     [aStream nextPutAll: '((usqInt) '.
>> > -                     self emitCExpression: rcvr on: aStream.
>> >                       shift < 0
>> > +                             ifTrue:
>> > +                                     [rightShift := TSendNode new
>> > +                                             setSelector: #>>
>> > +                                             receiver: msgNode receiver
>> > +                                             arguments: {TConstantNode
>> new setValue: shift negated}.
>> > +                                     self generateShiftRight:
>> rightShift on: aStream indent: level]
>> > +                             ifFalse: [self generateShiftLeft: msgNode
>> on: aStream indent: level].
>> > -                             ifTrue: [aStream nextPutAll: ' >> ';
>> print: shift negated]
>> > -                             ifFalse: [aStream nextPutAll: ' << ';
>> print: shift].
>> >                       aStream nextPut: $)]
>> >               ifFalse: "bit shift amount is an expression"
>> > +                     [rightShift := TSendNode new
>> > +                                             setSelector: #>>
>> > +                                             receiver: msgNode receiver
>> > +                                             arguments: {TSendNode new
>> > +                                                     setSelector:
>> #negated
>> > +                                                     receiver: arg
>> > +                                                     arguments: #()}.
>> > +                     aStream nextPutAll: '(('.
>> > +                     self emitCExpression: arg on: aStream.
>> > +                     aStream nextPutAll: ' < 0) ? ('.
>> > +                     self generateShiftRight: rightShift on: aStream
>> indent: level.
>> > +                     aStream nextPutAll: ') : ('.
>> > +                     self generateShiftLeft: msgNode on: aStream
>> indent: level.
>> > -                     [aStream nextPutAll: '(('.
>> > -                     self emitCExpression: arg on: aStream indent:
>> level.
>> > -                     aStream nextPutAll: ' < 0) ? ((usqInt) '.
>> > -                     self emitCExpression: rcvr on: aStream indent:
>> level.
>> > -                     aStream nextPutAll: ' >> -'.
>> > -                     self emitCExpression: arg on: aStream indent:
>> level.
>> > -                     aStream nextPutAll: ') : ((usqInt) '.
>> > -                     self emitCExpression: rcvr on: aStream indent:
>> level.
>> > -                     aStream nextPutAll: ' << '.
>> > -                     self emitCExpression: arg on: aStream indent:
>> level.
>> >                       aStream nextPutAll: '))']!
>> >
>> > Item was changed:
>> >  ----- Method: CCodeGenerator>>generateShiftRight:on:indent: (in
>> category 'C translation') -----
>> >  generateShiftRight: msgNode on: aStream indent: level
>> > +     "Generate the C code for this message onto the given stream.
>> > +     Note that this generates a Logical Shift (unsigned), not an
>> Arithmetic Shift (signed)."
>> > -     "Generate the C code for this message onto the given stream."
>> >
>> > +     | type typeIsUnsigned mustCastToUnsigned unsignedType |
>> > +     type := self typeFor: msgNode receiver in: currentMethod.
>> > +     typeIsUnsigned := type first = $u.
>> > +     mustCastToUnsigned := typeIsUnsigned not or:
>> > +             ["cast to usqInt if the int is shorter: we want to avoid
>> UB related to a shift exceeeding bit width"
>> > +             (self sizeOfIntegralCType: type) < (self
>> sizeOfIntegralCType: #usqInt)].
>> > +     "If not unsigned cast it to unsigned."
>> > +     mustCastToUnsigned
>> > -     | type |
>> > -     "If the variable is a 64-bit type then don't cast it to usqInt
>> (typically a 32-bit type)"
>> > -     (self is64BitIntegralVariable: msgNode receiver typeInto: [:t|
>> type := t])
>> >               ifTrue:
>> > +                     ["If the variable is a 64-bit type then don't
>> cast it to usqInt (typically a 32-bit type)"
>> > +                     unsignedType := (self sizeOfIntegralCType: type)
>> < (self sizeOfIntegralCType: #usqLong)
>> > +                             ifTrue: [#usqInt]
>> > +                             ifFalse: [self
>> unsignedTypeForIntegralType: type].
>> > +                      aStream nextPutAll: '(('; nextPutAll:
>> unsignedType; nextPutAll: ')('.
>> > -                     ["If not unsigned cast it to unsigned."
>> > -                      type first ~= $u ifTrue:
>> > -                             [aStream nextPutAll: '((unsigned ';
>> nextPutAll: type; nextPut: $)].
>> >                        self emitCExpression: msgNode receiver on:
>> aStream indent: level.
>> > +                      aStream nextPutAll: '))']
>> > -                      type first ~= $u ifTrue:
>> > -                             [aStream nextPut: $)]]
>> >               ifFalse:
>> > +                     [aStream nextPutAll: '('.
>> > -                     [aStream nextPutAll: '((usqInt) '.
>> >                        self emitCExpression: msgNode receiver on:
>> aStream indent: level.
>> >                        aStream nextPut: $)].
>> >       aStream nextPutAll: ' >> '.
>> >       self emitCExpression: msgNode args first on: aStream indent:
>> level!
>> >
>> > Item was changed:
>> >  ----- Method: CCodeGenerator>>generateSignedBitShift:on:indent: (in
>> category 'C translation') -----
>> >  generateSignedBitShift: msgNode on: aStream indent: level
>> >       "Generate the C code for this message onto the given stream."
>> >
>> > +     | arg shift rightShift |
>> > -     | cast type arg shift |
>> > -     "since ``signed'' is a synonym for ``signed int'' do not cast
>> 64-bit values to signed if at all possible."
>> > -     cast := (self is64BitIntegralVariable: msgNode receiver typeInto:
>> [:t| type := t])
>> > -                             ifTrue: ['(', (type first = $u ifTrue:
>> [type allButFirst: (type second = $n ifTrue: [2] ifFalse: [1])] ifFalse:
>> [type]), ')']
>> > -                             ifFalse: ['(signed)'].
>> >       (self isConstantNode: (arg := msgNode args first) valueInto:
>> [:shiftValue| shift := shiftValue])
>> >               ifTrue: "bit shift amount is a constant"
>> > +                     [aStream nextPut: $(.
>> > -                     [aStream nextPut: $(; nextPutAll: cast.
>> > -                     self emitCExpression: msgNode receiver on:
>> aStream.
>> >                       shift < 0
>> > +                             ifTrue:
>> > +                                     [rightShift := TSendNode new
>> > +                                             setSelector: #>>
>> > +                                             receiver: msgNode receiver
>> > +                                             arguments: {TConstantNode
>> new setValue: shift negated}.
>> > +                                     self generateSignedShiftRight:
>> rightShift on: aStream indent: level]
>> > +                             ifFalse: [self generateShiftLeft: msgNode
>> on: aStream indent: level].
>> > -                             ifTrue: [aStream nextPutAll: ' >> ';
>> print: shift negated]
>> > -                             ifFalse: [aStream nextPutAll: ' << ';
>> print: shift].
>> >                       aStream nextPut: $)]
>> >               ifFalse: "bit shift amount is an expression"
>> > +                     [rightShift := TSendNode new
>> > +                                             setSelector: #>>
>> > +                                             receiver: msgNode receiver
>> > +                                             arguments: {TSendNode new
>> > +                                                     setSelector:
>> #negated
>> > +                                                     receiver: arg
>> > +                                                     arguments: #()}.
>> > +                     aStream nextPutAll: '(('.
>> > -                     [aStream nextPutAll: '(('.
>> >                       self emitCExpression: arg on: aStream.
>> > +                     aStream nextPutAll: ' < 0) ? ('.
>> > +                     self generateSignedShiftRight: rightShift on:
>> aStream indent: level.
>> > +                     aStream nextPutAll: ') : ('.
>> > +                     self generateShiftLeft: msgNode on: aStream
>> indent: level.
>> > -                     aStream nextPutAll: ' < 0) ? ('; nextPutAll: cast.
>> > -                     self emitCExpression: msgNode receiver on:
>> aStream.
>> > -                     aStream nextPutAll: ' >> -'.
>> > -                     self emitCExpression: arg on: aStream.
>> > -                     aStream nextPutAll: ') : ('; nextPutAll: cast.
>> > -                     self emitCExpression: msgNode receiver on:
>> aStream.
>> > -                     aStream nextPutAll: ' << '.
>> > -                     self emitCExpression: arg on: aStream.
>> >                       aStream nextPutAll: '))']!
>> >
>> > Item was changed:
>> >  ----- Method: CCodeGenerator>>generateSignedShiftRight:on:indent: (in
>> category 'C translation') -----
>> >  generateSignedShiftRight: msgNode on: aStream indent: level
>> >       "Generate the C code for >>> onto the given stream."
>> >
>> > +     | type typeIsUnsigned mustCastToSigned signedType |
>> > +     type := self typeFor: msgNode receiver in: currentMethod.
>> > +     typeIsUnsigned := type first = $u.
>> > +     mustCastToSigned := typeIsUnsigned or:
>> > +             ["cast to sqInt if the int is shorter: we want to avoid
>> UB related to a shift exceeeding bit width"
>> > +             (self sizeOfIntegralCType: type) < (self
>> sizeOfIntegralCType: #usqInt)].
>> > +     mustCastToSigned
>> > -     (self is64BitIntegralVariable: msgNode receiver typeInto: [:t|])
>> >               ifTrue:
>> > +                     ["If the variable is a 64-bit type then don't
>> cast it to usqInt (typically a 32-bit type)"
>> > +                     signedType := (self sizeOfIntegralCType: type) <
>> (self sizeOfIntegralCType: #usqLong)
>> > +                             ifTrue: [#usqInt]
>> > +                             ifFalse: [self signedTypeForIntegralType:
>> type].
>> > +                      aStream nextPutAll: '(('; nextPutAll:
>> signedType; nextPutAll: ')('.
>> > +                      self emitCExpression: msgNode receiver on:
>> aStream indent: level.
>> > +                      aStream nextPutAll: '))']
>> > -                     [aStream nextPutAll: '((sqLong) ']
>> >               ifFalse:
>> > +                     [aStream nextPutAll: '('.
>> > +                      self emitCExpression: msgNode receiver on:
>> aStream indent: level.
>> > +                      aStream nextPut: $)].
>> > +     aStream nextPutAll: ' >> '.
>> > -                     [aStream nextPutAll: '((sqInt) '].
>> > -     self emitCExpression: msgNode receiver on: aStream.
>> > -     aStream nextPutAll: ') >> '.
>> >       self emitCExpression: msgNode args first on: aStream!
>>
>

-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20200310/14c3920c/attachment-0001.html>


More information about the Vm-dev mailing list