Hi Dave,
On Tue, 5 Apr 2016, David T. Lewis wrote:
> I just got around to checking this for code generation for the interpreter VM
> (VMMaker, not VMMaker.oscog). The #ifNil: is not translated at all, so I replaced
> it with this:
That's pretty interesting. The Cog branch behaves differently
matchTable ifNil: [
will become
if (!matchTable) {
while
matchTable = nil ifTrue: [
and
matchTable == nil ifTrue: [
will both compile to
if (matchTable == null) {
just like in the main branch.
All of these are correct, because null is just a macro to 0, but it's a
bit surprising that "ifNil:" and "== nil ifTrue:" are not the same in
Slang, while they are in Squeak.
My main concern is how matchTable is initialized. The generated code is
matchTable = arrayValueOf(stackValue(0));
matchTable -= 1;
if (failed()) {
return null;
}
which makes me think that matchTable will not be the C null pointer at
all when the argument nil is passed to the primitive, because
matchTable -= 1;
would not yield an unsigned char* with 0 value unless arrayValueOf
returns a pointer with the value 1 (assuming that sizeof(unsigned char) is
1).
I haven't found where arrayValueOf was defined, so I can't tell if it has
a chance to work at all.
Levente
P.S.: Since we're talking about C here, I CC'd the vm-dev list.
>
> matchTable = nil ifTrue:
>
> This produces the expected result, and can be compiled.
>
> There are other issues in the resulting VM, but they do not seem to be
> related to Collections-ul.685 (I reverted to Collections-ul.684 with the
> same results). I don't know the cause of those issues, but I will follow
> up in a separate thread as needed.
>
> With respect to Collections-ul.685, replacing the ifNil: test as above
> seems like the right thing to do.
>
> Dave
>
>
> On Sun, Apr 03, 2016 at 06:00:13PM -0400, David T. Lewis wrote:
>> Many people will not be familiar with this optimization, so a few words of
>> explanation are in order for background:
>>
>> There is a mechanism for translating certain methods in the image directly
>> into primitives for performance. This is normally not noticed, but you will
>> find occasional refernces such at this one in the image:
>>
>> findSubstring: key in: body startingAt: start matchTable: matchTable
>> <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>>
>> This is a method that first tries to call a version of itself that has
>> been translated to C. If the primitive fails, the normal Smalltalk method
>> is executed. The hidden tricky bit is that MiscPrimitivePlugin, which
>> lives in the VMMaker package, arranges for the #findSubstring:in:startingAt:matchTable:
>> method in the image to be translated to C and installed as a primitive.
>> This means that the generated VM is actually governed partly my methods
>> in the main image (not in the VMMaker package).
>>
>> It also means that if one of these methods is changed in the image, it
>> might accidentally result in unexpected (and possibly wrong) code being
>> generated for the plugin in the VM. That is the reason that Levente is
>> asking about the slang code generation in this case.
>>
>> Dave
>>
>>
>> On Sun, Apr 03, 2016 at 07:45:13PM +0200, Levente Uzonyi wrote:
>>> On Sun, 3 Apr 2016, commits(a)source.squeak.org wrote:
>>>
>>>> Levente Uzonyi uploaded a new version of Collections to project The Trunk:
>>>> http://source.squeak.org/trunk/Collections-ul.685.mcz
>>>>
>>>> ==================== Summary ====================
>>>>
>>>> Name: Collections-ul.685
>>>> Author: ul
>>>> Time: 3 April 2016, 3:21:51.035808 am
>>>> UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
>>>> Ancestors: Collections-eem.684
>>>>
>>>> Use Spur's new character comparison abilities in some String methods.
>>>>
>>>> =============== Diff against Collections-eem.684 ===============
>>>>
>>>> Item was changed:
>>>> ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in
>>>> category 'comparing') -----
>>>> findSubstring: key in: body startingAt: start matchTable: matchTable
>>>> "Answer the index in the string body at which the substring key
>>>> first occurs, at or beyond start. The match is determined using
>>>> matchTable, which can be used to effect, eg, case-insensitive
>>>> matches. If no match is found, zero will be returned.
>>>>
>>>> The algorithm below is not optimum -- it is intended to be
>>>> translated to C which will go so fast that it wont matter."
>>>> | index |
>>>> <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>>>> <var: #key declareC: 'unsigned char *key'>
>>>> <var: #body declareC: 'unsigned char *body'>
>>>> <var: #matchTable declareC: 'unsigned char *matchTable'>
>>>>
>>>> key size = 0 ifTrue: [^ 0].
>>>> + matchTable ifNil: [
>>>> + start to: body size - key size + 1 do: [ :startIndex |
>>>> + index := 0.
>>>> + [ (body at: startIndex + index) == (key at: (index
>>>> := index + 1)) ] whileTrue: [
>>>> + index = key size ifTrue: [ ^startIndex ] ] ].
>>>> + ^0 ].
>>>
>>> I wonder if the #ifNil: check is correct in Slang. The generated code is
>>>
>>> ...
>>> unsigned char *matchTable;
>>> ...
>>> matchTable = arrayValueOf(stackValue(0));
>>> matchTable -= 1;
>>> ...
>>> if (!(matchTable)) {
>>> ...
>>>
>>> So, will the value of matchTable actually be NULL or not, when nil is
>>> passed to the primitive. That -= 1 makes me think that it won't be NULL.
>>>
>>> Another thing is that the loop condition, while it's order of evaluation
>>> is well defined in Smalltalk, will compile to C code containing undefined
>>> behavior:
>>>
>>> while ((body[startIndex + index]) == (key[index +=
>>> 1])) {
>>>
>>> Since in C the evaluation of the left and right side of == can happen in
>>> any order, the generated machine code may or may not be correct.
>>> Is this is a difference between Smalltalk and Slang, or is it just a slip
>>> in the code generator?
>>>
>>> Levente
>>>
>>>> (start max: 1) to: body size - key size + 1 do:
>>>> [:startIndex |
>>>> index := 1.
>>>> [(matchTable at: (body at: startIndex+index-1)
>>>> asciiValue + 1)
>>>> = (matchTable at: (key at: index) asciiValue
>>>> + 1)]
>>>> whileTrue:
>>>> [index = key size ifTrue: [^ startIndex].
>>>> index := index+1]].
>>>> ^ 0
>>>> "
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 1 matchTable:
>>>> CaseSensitiveOrder 1
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 2 matchTable:
>>>> CaseSensitiveOrder 7
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 8 matchTable:
>>>> CaseSensitiveOrder 0
>>>> ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
>>>> CaseSensitiveOrder 0
>>>> ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
>>>> CaseInsensitiveOrder 7
>>>> "!
>
>
Eliot Miranda uploaded a new version of VMMaker to project VM Maker:
http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1780.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1780
Author: eem
Time: 7 April 2016, 5:29:12.636568 pm
UUID: 4b90904a-80a2-4646-adc3-e5496b2fd317
Ancestors: VMMaker.oscog-eem.1779
Eliminate duplicate initialization of the CoInterpreter hierarchy.
=============== Diff against VMMaker.oscog-eem.1779 ===============
Item was changed:
----- Method: CogVMSimulator class>>initializeWithOptions:objectMemoryClass: (in category 'class initialization') -----
initializeWithOptions: optionsDictionaryOrArray objectMemoryClass: objectMemoryClassOrNil
"The relevant ObjectMemory, Interpreter and Cogit classes must be initialized in order.
This happens notionally every time we start the simulator,
but in fact happens when ever we instantiate a simulator."
initializationOptions := optionsDictionaryOrArray isArray
ifTrue: [Dictionary newFromPairs: optionsDictionaryOrArray]
ifFalse: [optionsDictionaryOrArray].
(objectMemoryClassOrNil ifNil: [self objectMemoryClass])
initializeWithOptions: initializationOptions.
- self initializeWithOptions: initializationOptions.
-
((initializationOptions at: #COGMTVM ifAbsent: [false])
ifTrue: [CoInterpreterMT]
ifFalse: [CoInterpreter])
initializeWithOptions: initializationOptions.
ByteCountsPerMicrosecond := initializationOptions
at: #ByteCountsPerMicrosecond
ifAbsent: [100].
(self cogitClass withAllSuperclasses copyUpTo: Cogit) reverseDo:
[:c| c initializeWithOptions: initializationOptions]!
Ben, Nicolas, you've raised important issues in recent messages. Alas my internet connexion is down and I can't respond until it's back. Forgive the blackout period. "It'll be a few hours", said support...
_,,,^..^,,,_ (phone)
Just notice in Squeak 5.0 I tried loading VMMaker.oscog-eem.1778
and got Warning: This package depends on the following classes:
FFIConstants
KlattResonatorIndices
cheers -ben
Eliot Miranda uploaded a new version of VMMaker to project VM Maker:
http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1777.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1777
Author: eem
Time: 6 April 2016, 6:31:28.51167 pm
UUID: a1efdb25-0231-43f2-bf60-5e6eb78dbf3a
Ancestors: VMMaker.oscog-eem.1776
Simulator:
Rationalize cCoerce:to:. Put one implementation in VMClass and override only in the Cogit (which defers to objectMemory) and InterpreterPlugin which simply answers the value.
N.B. This means that in the InterpreterPlugin hierarchy if you want null coercion (as for example in BalloonEngineBase>>initColorTransform) you sent cCoerce:to: to self, but if you want the proper coercion of oops you /must/ send cCoerce:to: to interpreterProxy.
Fix just such a coercion slip in LargeIntegersPlugin>>digitAddLarge:with:
=============== Diff against VMMaker.oscog-eem.1776 ===============
Item was removed:
- ----- Method: CogVMSimulator>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion for translation only; just return the value when running in Smalltalk."
-
- ^value == nil
- ifTrue: [value]
- ifFalse: [value coerceTo: cTypeString sim: self]!
Item was added:
+ ----- Method: InterpreterPlugin>>cCoerce:to: (in category 'simulation') -----
+ cCoerce: value to: cType
+ "Type coercion for translation only; just return the value when running in Smalltalk.
+ This overrides the generic coercion method in VMClass. For some reason we are the exception.
+ If we want that style of coercion we can send cCoerce:to: to interpreterProxy, not self."
+
+ ^value!
Item was removed:
- ----- Method: InterpreterSimulator>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion for translation only; just return the value when running in Smalltalk."
-
- ^value == nil
- ifTrue: [value]
- ifFalse: [value coerceTo: cTypeString sim: self]!
Item was changed:
----- Method: LargeIntegersPlugin>>digitAddLarge:with: (in category 'oop functions') -----
digitAddLarge: firstInteger with: secondInteger
"Does not need to normalize!!"
| over firstDigitLen secondDigitLen shortInt shortDigitLen longInt longDigitLen sum newSum neg |
<var: #over type: #'unsigned int'>
firstDigitLen := self digitSizeOfLargeInt: firstInteger.
secondDigitLen := self digitSizeOfLargeInt: secondInteger.
neg := (interpreterProxy fetchClassOf: firstInteger)
= interpreterProxy classLargeNegativeInteger.
firstDigitLen <= secondDigitLen
ifTrue:
[shortInt := firstInteger.
shortDigitLen := firstDigitLen.
longInt := secondInteger.
longDigitLen := secondDigitLen]
ifFalse:
[shortInt := secondInteger.
shortDigitLen := secondDigitLen.
longInt := firstInteger.
longDigitLen := firstDigitLen].
" sum := Integer new: len neg: firstInteger negative."
self remapOop: #(shortInt longInt ) in: [sum := self createLargeIntegerNeg: neg digitLength: longDigitLen].
over := self
cDigitAdd: (interpreterProxy firstIndexableField: shortInt)
len: shortDigitLen
with: (interpreterProxy firstIndexableField: longInt)
len: longDigitLen
into: (interpreterProxy firstIndexableField: sum).
over > 0
ifTrue:
["sum := sum growby: 1."
self remapOop: sum in: [newSum := self createLargeIntegerNeg: neg byteLength: longDigitLen * 4 + 1].
self
cDigitCopyFrom: (interpreterProxy firstIndexableField: sum)
to: (interpreterProxy firstIndexableField: newSum)
len: longDigitLen.
sum := newSum.
"C index!!"
+ self cDigitOf: (interpreterProxy cCoerce: (interpreterProxy firstIndexableField: sum) to: 'unsigned int *')
- self cDigitOf: (self cCoerce: (interpreterProxy firstIndexableField: sum) to: 'unsigned int *')
at: longDigitLen put: over]
ifFalse:
[sum := neg
ifTrue: [self normalizeNegative: sum]
ifFalse: [self normalizePositive: sum]].
^ sum!
Item was removed:
- ----- Method: NewCoObjectMemorySimulator>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion for translation only; just return the value when running in Smalltalk."
-
- ^value == nil
- ifTrue: [value]
- ifFalse: [value coerceTo: cTypeString sim: self]!
Item was removed:
- ----- Method: NewObjectMemorySimulator>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion for translation only; just return the value when running in Smalltalk."
-
- ^value == nil
- ifTrue: [value]
- ifFalse: [value coerceTo: cTypeString sim: self]!
Item was removed:
- ----- Method: SpurMemoryManager>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion. For translation a cast will be emmitted. When running in Smalltalk
- answer a suitable wrapper for correct indexing."
- <doNotGenerate>
- ^value
- ifNil: [value]
- ifNotNil: [value coerceTo: cTypeString sim: self]!
Item was removed:
- ----- Method: StackInterpreterSimulator>>cCoerce:to: (in category 'memory access') -----
- cCoerce: value to: cTypeString
- "Type coercion for translation only; just return the value when running in Smalltalk."
-
- ^value == nil
- ifTrue: [value]
- ifFalse: [value coerceTo: cTypeString sim: self]!
Item was added:
+ ----- Method: VMClass>>cCoerce:to: (in category 'memory access') -----
+ cCoerce: value to: cTypeString
+ "Type coercion. For translation a cast will be emmitted. When running in Smalltalk
+ answer a suitable wrapper for correct indexing."
+ <doNotGenerate>
+ ^value
+ ifNil: [value]
+ ifNotNil: [value coerceTo: cTypeString sim: self]!
Eliot Miranda uploaded a new version of VMMaker to project VM Maker:
http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1775.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1775
Author: eem
Time: 6 April 2016, 5:43:29.382334 pm
UUID: 643dcc90-05cd-4945-9531-0bb9538805e0
Ancestors: VMMaker.oscog-cb.1774
Simulator:
Set SPURVM correctly on initialization. Old code would always set it to false.
=============== Diff against VMMaker.oscog-cb.1774 ===============
Item was changed:
----- Method: SpurMemoryManager class>>initBytesPerWord: (in category 'class initialization') -----
initBytesPerWord: wordSize
BytesPerWord := BytesPerOop := wordSize.
"N.B. This is *not* output when generating the interpreter file.
It is left to the various sqConfig.h files to define correctly."
+ VMBIGENDIAN := Smalltalk endianness == #big.
+ SPURVM := true!
- VMBIGENDIAN := Smalltalk endianness == #big!
Item was changed:
----- Method: VMClass class>>initializeMiscConstants (in category 'initialization') -----
initializeMiscConstants
"Falsify the `what type of VM is this?' flags that are defined in the various interp.h files.
Subclass implementations need to include a super initializeMiscConstants"
| omc |
VMBIGENDIAN class. "Mention this for the benefit of CCodeGenerator>>emitCConstantsOn:"
SPURVM := STACKVM := COGVM := COGMTVM := false.
initializationOptions ifNil: [self initializationOptions: Dictionary new].
omc := initializationOptions at: #ObjectMemory ifAbsent: nil.
(omc isNil and: [self defaultObjectMemoryClass notNil]) ifTrue:
[omc := initializationOptions at: #ObjectMemory put: self defaultObjectMemoryClass name].
initializationOptions
at: #SqueakV3ObjectMemory "the good ole default"
ifAbsentPut: (omc
ifNil: [true]
ifNotNil: [(Smalltalk at: omc) includesBehavior: ObjectMemory]);
at: #SpurObjectMemory "the new contender"
ifAbsentPut: (omc
ifNil: [false]
ifNotNil: [(Smalltalk at: omc) includesBehavior: SpurMemoryManager]).
"Use ifAbsentPut: so that they will get copied back to the
VMMaker's options and dead code will likely be eliminated."
PharoVM := initializationOptions at: #PharoVM ifAbsentPut: [false].
NewspeakVM := initializationOptions at: #NewspeakVM ifAbsentPut: [false].
SistaVM := initializationOptions at: #SistaVM ifAbsentPut: [false].
"But not these; they're compile-time"
MULTIPLEBYTECODESETS := initializationOptions at: #MULTIPLEBYTECODESETS ifAbsent: [false].
IMMUTABILITY := initializationOptions at: #IMMUTABILITY ifAbsent: [false].
"These must be set only if specified, not defaulted, because they are set on the command line or in include files."
initializationOptions
at: #VMBIGENDIAN ifPresent: [:value| VMBIGENDIAN := value];
+ at: #ObjectMemory ifPresent: [:value| SPURVM := value beginsWith: 'Spur'];
at: #STACKVM ifPresent: [:value| STACKVM := value];
at: #COGVM ifPresent: [:value| COGVM := initializationOptions at: #COGVM];
at: #COGMTVM ifPresent: [:value| COGMTVM := initializationOptions at: #COGMTVM]!