<div dir="ltr"><div dir="ltr">Hi Nicolas,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 10, 2020 at 2:15 PM Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> <div dir="ltr"><div>Hi Levente,</div><div>for some reason, early VM writers were looking for a Logical Right Shift (not propagating the sign bit).</div><div>So that's how bitShift: (then >>) were historically translated - no matter how weird or contradictory to VM simulation it can be.</div><div>I guess that it is of greatest interest for generating BitBlt operations (Simulation works ok because WordArray are like unsigned).</div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>Eliot had to later introduce >>> for Arithmetic Right Shift and signedBitShift: too, because we sometimes need those operations too.<br></div><div>That makes one more surprise because we now have the exact opposite of Java semantics for >> and >>> !</div></div></blockquote><div><br></div><div>:-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>We have another similar surprising behavior with translation of // and \\ which are translated into C operations equivalent to quo: and rem: <br></div><div><a href="https://smallissimo.blogspot.com/2015/03/is-bitshift-equivalent-to-division-in.html?m=0" target="_blank">https://smallissimo.blogspot.com/2015/03/is-bitshift-equivalent-to-division-in.html?m=0</a></div><div><br></div><div>These hackish choices are unfortunate, but very difficult to change now without hiccups...<br></div></div></blockquote><div><br></div><div>+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 ;-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le mar. 10 mars 2020 à 21:55, Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu" target="_blank">leves@caesar.elte.hu</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> <br>
Hi Nicolas,<br>
<br>
Thanks for the fix. It works.<br>
I wonder why is it necessary to cast to usqInt. Is it just to ensure <br>
unsignedness?<br>
<br>
<br>
Levente<br>
<br>
On Mon, 9 Mar 2020, <a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a> wrote:<br>
<br>
> <br>
> Nicolas Cellier uploaded a new version of VMMaker to project VM Maker:<br>
> <a href="http://source.squeak.org/VMMaker/VMMaker.oscog-nice.2723.mcz" rel="noreferrer" target="_blank">http://source.squeak.org/VMMaker/VMMaker.oscog-nice.2723.mcz</a><br>
><br>
> ==================== Summary ====================<br>
><br>
> Name: VMMaker.oscog-nice.2723<br>
> Author: nice<br>
> Time: 10 March 2020, 12:26:31.04183 am<br>
> UUID: c1319382-406c-43a7-9f55-2b48c4007d80<br>
> Ancestors: VMMaker.oscog-eem.2722<br>
><br>
> Fix the right shift: dont convert to usqInt a type longer than usqInt<br>
><br>
> =============== Diff against VMMaker.oscog-eem.2722 ===============<br>
><br>
> Item was changed:<br>
> ----- Method: CCodeGenerator>>generateBitShift:on:indent: (in category 'C translation') -----<br>
> generateBitShift: msgNode on: aStream indent: level<br>
> "Generate the C code for this message onto the given stream."<br>
> <br>
> + | arg shift rightShift |<br>
> + (self isConstantNode: (arg := msgNode args first) valueInto: [:shiftValue| shift := shiftValue])<br>
> - | arg rcvr shift |<br>
> - arg := msgNode args first.<br>
> - rcvr := msgNode receiver.<br>
> - (self isConstantNode: arg valueInto: [:shiftValue| shift := shiftValue])<br>
> ifTrue: "bit shift amount is a constant"<br>
> + [aStream nextPut: $(.<br>
> - [aStream nextPutAll: '((usqInt) '.<br>
> - self emitCExpression: rcvr on: aStream.<br>
> shift < 0<br>
> + ifTrue:<br>
> + [rightShift := TSendNode new<br>
> + setSelector: #>><br>
> + receiver: msgNode receiver<br>
> + arguments: {TConstantNode new setValue: shift negated}.<br>
> + self generateShiftRight: rightShift on: aStream indent: level]<br>
> + ifFalse: [self generateShiftLeft: msgNode on: aStream indent: level].<br>
> - ifTrue: [aStream nextPutAll: ' >> '; print: shift negated]<br>
> - ifFalse: [aStream nextPutAll: ' << '; print: shift].<br>
> aStream nextPut: $)]<br>
> ifFalse: "bit shift amount is an expression"<br>
> + [rightShift := TSendNode new<br>
> + setSelector: #>><br>
> + receiver: msgNode receiver<br>
> + arguments: {TSendNode new<br>
> + setSelector: #negated<br>
> + receiver: arg<br>
> + arguments: #()}.<br>
> + aStream nextPutAll: '(('.<br>
> + self emitCExpression: arg on: aStream.<br>
> + aStream nextPutAll: ' < 0) ? ('.<br>
> + self generateShiftRight: rightShift on: aStream indent: level. <br>
> + aStream nextPutAll: ') : ('.<br>
> + self generateShiftLeft: msgNode on: aStream indent: level.<br>
> - [aStream nextPutAll: '(('.<br>
> - self emitCExpression: arg on: aStream indent: level.<br>
> - aStream nextPutAll: ' < 0) ? ((usqInt) '.<br>
> - self emitCExpression: rcvr on: aStream indent: level.<br>
> - aStream nextPutAll: ' >> -'.<br>
> - self emitCExpression: arg on: aStream indent: level.<br>
> - aStream nextPutAll: ') : ((usqInt) '.<br>
> - self emitCExpression: rcvr on: aStream indent: level.<br>
> - aStream nextPutAll: ' << '.<br>
> - self emitCExpression: arg on: aStream indent: level.<br>
> aStream nextPutAll: '))']!<br>
><br>
> Item was changed:<br>
> ----- Method: CCodeGenerator>>generateShiftRight:on:indent: (in category 'C translation') -----<br>
> generateShiftRight: msgNode on: aStream indent: level<br>
> + "Generate the C code for this message onto the given stream.<br>
> + Note that this generates a Logical Shift (unsigned), not an Arithmetic Shift (signed)."<br>
> - "Generate the C code for this message onto the given stream."<br>
> <br>
> + | type typeIsUnsigned mustCastToUnsigned unsignedType |<br>
> + type := self typeFor: msgNode receiver in: currentMethod.<br>
> + typeIsUnsigned := type first = $u.<br>
> + mustCastToUnsigned := typeIsUnsigned not or:<br>
> + ["cast to usqInt if the int is shorter: we want to avoid UB related to a shift exceeeding bit width"<br>
> + (self sizeOfIntegralCType: type) < (self sizeOfIntegralCType: #usqInt)].<br>
> + "If not unsigned cast it to unsigned."<br>
> + mustCastToUnsigned<br>
> - | type |<br>
> - "If the variable is a 64-bit type then don't cast it to usqInt (typically a 32-bit type)"<br>
> - (self is64BitIntegralVariable: msgNode receiver typeInto: [:t| type := t])<br>
> ifTrue:<br>
> + ["If the variable is a 64-bit type then don't cast it to usqInt (typically a 32-bit type)"<br>
> + unsignedType := (self sizeOfIntegralCType: type) < (self sizeOfIntegralCType: #usqLong)<br>
> + ifTrue: [#usqInt]<br>
> + ifFalse: [self unsignedTypeForIntegralType: type].<br>
> + aStream nextPutAll: '(('; nextPutAll: unsignedType; nextPutAll: ')('.<br>
> - ["If not unsigned cast it to unsigned."<br>
> - type first ~= $u ifTrue:<br>
> - [aStream nextPutAll: '((unsigned '; nextPutAll: type; nextPut: $)].<br>
> self emitCExpression: msgNode receiver on: aStream indent: level.<br>
> + aStream nextPutAll: '))']<br>
> - type first ~= $u ifTrue:<br>
> - [aStream nextPut: $)]]<br>
> ifFalse:<br>
> + [aStream nextPutAll: '('.<br>
> - [aStream nextPutAll: '((usqInt) '.<br>
> self emitCExpression: msgNode receiver on: aStream indent: level.<br>
> aStream nextPut: $)].<br>
> aStream nextPutAll: ' >> '.<br>
> self emitCExpression: msgNode args first on: aStream indent: level!<br>
><br>
> Item was changed:<br>
> ----- Method: CCodeGenerator>>generateSignedBitShift:on:indent: (in category 'C translation') -----<br>
> generateSignedBitShift: msgNode on: aStream indent: level<br>
> "Generate the C code for this message onto the given stream."<br>
> <br>
> + | arg shift rightShift |<br>
> - | cast type arg shift |<br>
> - "since ``signed'' is a synonym for ``signed int'' do not cast 64-bit values to signed if at all possible."<br>
> - cast := (self is64BitIntegralVariable: msgNode receiver typeInto: [:t| type := t])<br>
> - ifTrue: ['(', (type first = $u ifTrue: [type allButFirst: (type second = $n ifTrue: [2] ifFalse: [1])] ifFalse: [type]), ')']<br>
> - ifFalse: ['(signed)'].<br>
> (self isConstantNode: (arg := msgNode args first) valueInto: [:shiftValue| shift := shiftValue])<br>
> ifTrue: "bit shift amount is a constant"<br>
> + [aStream nextPut: $(.<br>
> - [aStream nextPut: $(; nextPutAll: cast.<br>
> - self emitCExpression: msgNode receiver on: aStream.<br>
> shift < 0<br>
> + ifTrue:<br>
> + [rightShift := TSendNode new<br>
> + setSelector: #>><br>
> + receiver: msgNode receiver<br>
> + arguments: {TConstantNode new setValue: shift negated}.<br>
> + self generateSignedShiftRight: rightShift on: aStream indent: level]<br>
> + ifFalse: [self generateShiftLeft: msgNode on: aStream indent: level].<br>
> - ifTrue: [aStream nextPutAll: ' >> '; print: shift negated]<br>
> - ifFalse: [aStream nextPutAll: ' << '; print: shift].<br>
> aStream nextPut: $)]<br>
> ifFalse: "bit shift amount is an expression"<br>
> + [rightShift := TSendNode new<br>
> + setSelector: #>><br>
> + receiver: msgNode receiver<br>
> + arguments: {TSendNode new<br>
> + setSelector: #negated<br>
> + receiver: arg<br>
> + arguments: #()}.<br>
> + aStream nextPutAll: '(('.<br>
> - [aStream nextPutAll: '(('.<br>
> self emitCExpression: arg on: aStream.<br>
> + aStream nextPutAll: ' < 0) ? ('.<br>
> + self generateSignedShiftRight: rightShift on: aStream indent: level. <br>
> + aStream nextPutAll: ') : ('.<br>
> + self generateShiftLeft: msgNode on: aStream indent: level.<br>
> - aStream nextPutAll: ' < 0) ? ('; nextPutAll: cast.<br>
> - self emitCExpression: msgNode receiver on: aStream.<br>
> - aStream nextPutAll: ' >> -'.<br>
> - self emitCExpression: arg on: aStream.<br>
> - aStream nextPutAll: ') : ('; nextPutAll: cast.<br>
> - self emitCExpression: msgNode receiver on: aStream.<br>
> - aStream nextPutAll: ' << '.<br>
> - self emitCExpression: arg on: aStream.<br>
> aStream nextPutAll: '))']!<br>
><br>
> Item was changed:<br>
> ----- Method: CCodeGenerator>>generateSignedShiftRight:on:indent: (in category 'C translation') -----<br>
> generateSignedShiftRight: msgNode on: aStream indent: level<br>
> "Generate the C code for >>> onto the given stream."<br>
> <br>
> + | type typeIsUnsigned mustCastToSigned signedType |<br>
> + type := self typeFor: msgNode receiver in: currentMethod.<br>
> + typeIsUnsigned := type first = $u.<br>
> + mustCastToSigned := typeIsUnsigned or:<br>
> + ["cast to sqInt if the int is shorter: we want to avoid UB related to a shift exceeeding bit width"<br>
> + (self sizeOfIntegralCType: type) < (self sizeOfIntegralCType: #usqInt)].<br>
> + mustCastToSigned<br>
> - (self is64BitIntegralVariable: msgNode receiver typeInto: [:t|])<br>
> ifTrue:<br>
> + ["If the variable is a 64-bit type then don't cast it to usqInt (typically a 32-bit type)"<br>
> + signedType := (self sizeOfIntegralCType: type) < (self sizeOfIntegralCType: #usqLong)<br>
> + ifTrue: [#usqInt]<br>
> + ifFalse: [self signedTypeForIntegralType: type].<br>
> + aStream nextPutAll: '(('; nextPutAll: signedType; nextPutAll: ')('.<br>
> + self emitCExpression: msgNode receiver on: aStream indent: level.<br>
> + aStream nextPutAll: '))']<br>
> - [aStream nextPutAll: '((sqLong) ']<br>
> ifFalse:<br>
> + [aStream nextPutAll: '('.<br>
> + self emitCExpression: msgNode receiver on: aStream indent: level.<br>
> + aStream nextPut: $)].<br>
> + aStream nextPutAll: ' >> '.<br>
> - [aStream nextPutAll: '((sqInt) '].<br>
> - self emitCExpression: msgNode receiver on: aStream.<br>
> - aStream nextPutAll: ') >> '.<br>
> self emitCExpression: msgNode args first on: aStream!<br>
</blockquote></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div></div>