<div dir="ltr">Hi Nicolas,<div><br></div><div> I think it's when the already inlined positive32BitIntegerFor: gets inlined into positiveMachineIntegerFor:. The code looks correct once maybeInlinePositive32BitIntegerFor: is inlined into positive32BitIntegerFor:, but not after inlining that into positiveMachineIntegerFor:. Here's the doit I use to debug this:</div><div><br></div><div><div>Transcript show: [| sel vmm s cg |</div><div><span class="" style="white-space:pre">        </span>sel := #sendInvokeCallbackContext:.</div><div><span class="" style="white-space:pre">        </span>vmm := (VMMaker forPlatform: 'Cross')</div><div><span class="" style="white-space:pre">                                </span>interpreterClass: StackInterpreter;</div><div><span class="" style="white-space:pre">                                </span>options: #(ObjectMemory Spur32BitMemoryManager "Spur64BitMemoryManager"</div><div><span class="" style="white-space:pre">                                                        </span>"NewspeakVM true MULTIPLEBYTECODESETS true"</div><div><span class="" style="white-space:pre">                                                        </span>IMMUTABILITY false).</div><div><span class="" style="white-space:pre">        </span>cg := [vmm buildCodeGeneratorForInterpreter]</div><div><span class="" style="white-space:pre">                        </span>on: Notification</div><div><span class="" style="white-space:pre">                        </span>do: [:ex|</div><div><span class="" style="white-space:pre">                                </span>ex tag == #getVMMaker</div><div><span class="" style="white-space:pre">                                        </span>ifTrue: [ex resume: vmm]</div><div><span class="" style="white-space:pre">                                        </span>ifFalse: [ex pass]].</div><div><span class="" style="white-space:pre">        </span>cg<span class="" style="white-space:pre">        </span>breakSrcInlineSelector: #positive32BitIntegerFor:"positiveMachineIntegerFor:";</div><div><span class="" style="white-space:pre">                </span>breakDestInlineSelector: #positiveMachineIntegerFor:"sel";</div><div><span class="" style="white-space:pre">                </span>breakOnInline: "false"true.</div><div><span class="" style="white-space:pre">        </span>cg vmClass preGenerationHook: cg.</div><div><span class="" style="white-space:pre">        </span>cg inferTypesForImplicitlyTypedVariablesAndMethods.</div><div><span class="" style="white-space:pre">        </span>cg retainMethods: { sel }.</div><div><span class="" style="white-space:pre">        </span>cg prepareMethods.</div><div><span class="" style="white-space:pre">        </span>((sel beginsWith: 'bytecode') or: [sel endsWith: 'Bytecode'])</div><div><span class="" style="white-space:pre">                </span>ifTrue: [cg doBasicInlining: true]</div><div><span class="" style="white-space:pre">                </span>ifFalse: [cg doInlining: true].</div><div><span class="" style="white-space:pre">        </span>s := ReadWriteStream on: String new.</div><div><span class="" style="white-space:pre">        </span>(cg methodNamed: sel)</div><div><span class="" style="white-space:pre">                </span>removeUnusedTempsAndNilIfRequiredIn: cg;</div><div><span class="" style="white-space:pre">                </span>halt;</div><div><span class="" style="white-space:pre">                </span>emitCCodeOn: s generator: cg.</div><div><span class="" style="white-space:pre">        </span>s contents] value</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 27, 2016 at 3:11 PM, Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br><div dir="ltr"><div><div>It sounds like related to ifTrue:ifFalse: alternatives.<br></div>When entering pass 6 inlined is still correct:<br><br> "begin push:".<br> objectMemory wordSize = 8 ifTrue: [object3 := self positive64BitIntegerFor: vmCallbackContext asUnsignedInteger] ifFalse: [<br> "begin positive32BitIntegerFor:".<br> false ifTrue: [<br> object3 := objectMemory integerObjectOf: (vmCallbackContext asUnsignedInteger asUnsignedLong bitAnd: 4294967295).<br> goto l1<br> ] ifFalse: [<br> object3 := self maybeInlinePositive32BitIntegerFor: vmCallbackContext asUnsignedInteger.<br> goto l1<br> ].<br> l1: "end positive32BitIntegerFor:"<br> ].<br> stackPages longAt: sp4 := stackPointer - objectMemory wordSize put: object3.<br> stackPointer := sp4.<br><br></div><div>The bad step happens before entering in pass 7. It correctly applies to above instance of this message send with object3 := assignment distributed in each ifTrue:ifFalse: returning branch:<br><br>objectMemory wordSize = 8 ifTrue: [object3 := self positive64BitIntegerFor: vmCallbackContext asUnsignedInteger] ifFalse: [<br> "begin positive32BitIntegerFor:".<br> false ifTrue: [<br> object3 := objectMemory integerObjectOf: (vmCallbackContext asUnsignedInteger asUnsignedLong bitAnd: 4294967295).<br> goto l1<br> ] ifFalse: [<br> "begin maybeInlinePositive32BitIntegerFor:".<br> self deny: objectMemory hasSixtyFourBitImmediates.<br> (self cCoerceSimple: vmCallbackContext asUnsignedInteger to: unsigned int) <= objectMemory maxSmallInteger ifTrue: [<br> object3 := objectMemory integerObjectOf: vmCallbackContext asUnsignedInteger.<br> goto l20<br> ].<br> "begin eeInstantiateSmallClassIndex:format:numSlots:".<br> objFormat3 := self firstByteFormat + (8 - 4 bitAnd: self wordSize - 1).<br> self assert: (1 >= 0 and: [ClassLargePositiveIntegerCompactIndex ~= 0]).<br> self assert: (objFormat3 < self firstByteFormat ifTrue: [objFormat3] ifFalse: [objFormat3 bitAnd: self byteFormatMask]) = (self instSpecOfClass: (self knownClassAtIndex: ClassLargePositiveIntegerCompactIndex)).<br> "begin allocateSmallNewSpaceSlots:format:classIndex:".<br> self assert: 1 < self numSlotsMask.<br> newObj3 := freeStart.<br> numBytes3 := self baseHeaderSize + (1 <= 1 ifTrue: [8] ifFalse: [1 + (1 bitAnd: 1) * self bytesPerOop]).<br> self assert: numBytes3 \\ self allocationUnit = 0.<br> self assert: newObj3 \\ self allocationUnit = 0.<br> freeStart + numBytes3 > scavengeThreshold ifTrue: [<br> needGCFlag ifFalse: [<br> "begin scheduleScavenge".<br> needGCFlag := true.<br> coInterpreter forceInterruptCheck<br> ].<br> freeStart + numBytes3 > scavenger eden limit ifTrue: [<br> self error: 'no room in eden for allocateSmallNewSpaceSlots:format:classIndex:'.<br> newLargeInteger3 := 0.<br> goto l19<br> ]<br> ].<br> self long64At: newObj3 put: (self cCoerceSimple: 1 to: usqLong) << self numSlotsFullShift + objFormat3 << self formatShift + ClassLargePositiveIntegerCompactIndex.<br> freeStart := freeStart + numBytes3.<br> newLargeInteger3 := newObj3.<br> l19: "end allocateSmallNewSpaceSlots:format:classIndex:".<br> self cppIf: SPURVM ifTrue: [<br> self long32At: newLargeInteger3 + self baseHeaderSize + 0 << 2 put: (objectMemory byteSwapped32IfBigEndian: vmCallbackContext asUnsignedInteger).<br> self long32At: newLargeInteger3 + self baseHeaderSize + 1 << 2 put: 0<br> ] ifFalse: [self long32At: newLargeInteger3 + self baseHeaderSize + 0 << 2 put: (objectMemory byteSwapped32IfBigEndian: vmCallbackContext asUnsignedInteger)].<br> object3 := newLargeInteger3.<br> l20: "end maybeInlinePositive32BitIntegerFor:".<br> goto l1<br> ].<br> l1: "end positive32BitIntegerFor:"<br> ]<br><br></div><div>However, at same stage, the transformation also applies to previous instance of same message, but this time without correct distribution of object := assignment.<br></div><div><br> "begin push:".<br> "begin positiveMachineIntegerFor:".<br> value := vmCallbackContext thunkp asUnsignedInteger.<br> object := objectMemory wordSize = 8 ifTrue: [self positive64BitIntegerFor: value] ifFalse: [<br> "begin positive32BitIntegerFor:".<br> false ifTrue: [<br> objectMemory integerObjectOf: (value asUnsignedLong bitAnd: 4294967295).<br> goto l5<br> ] ifFalse: [<br> "begin maybeInlinePositive32BitIntegerFor:".<br> self deny: objectMemory hasSixtyFourBitImmediates.<br> (self cCoerceSimple: value to: unsigned int) <= objectMemory maxSmallInteger ifTrue: [<br> objectMemory integerObjectOf: value.<br> goto l4<br> ].<br> "begin eeInstantiateSmallClassIndex:format:numSlots:".<br> objFormat := self firstByteFormat + (8 - 4 bitAnd: self wordSize - 1).<br> self assert: (1 >= 0 and: [ClassLargePositiveIntegerCompactIndex ~= 0]).<br> self assert: (objFormat < self firstByteFormat ifTrue: [objFormat] ifFalse: [objFormat bitAnd: self byteFormatMask]) = (self instSpecOfClass: (self knownClassAtIndex: ClassLargePositiveIntegerCompactIndex)).<br> "begin allocateSmallNewSpaceSlots:format:classIndex:".<br> self assert: 1 < self numSlotsMask.<br> newObj := freeStart.<br> numBytes := self baseHeaderSize + (1 <= 1 ifTrue: [8] ifFalse: [1 + (1 bitAnd: 1) * self bytesPerOop]).<br> self assert: numBytes \\ self allocationUnit = 0.<br> self assert: newObj \\ self allocationUnit = 0.<br> freeStart + numBytes > scavengeThreshold ifTrue: [<br> needGCFlag ifFalse: [<br> "begin scheduleScavenge".<br> needGCFlag := true.<br> coInterpreter forceInterruptCheck<br> ].<br> freeStart + numBytes > scavenger eden limit ifTrue: [<br> self error: 'no room in eden for allocateSmallNewSpaceSlots:format:classIndex:'.<br> newLargeInteger := 0.<br> goto l6<br> ]<br> ].<br> self long64At: newObj put: (self cCoerceSimple: 1 to: usqLong) << self numSlotsFullShift + objFormat << self formatShift + ClassLargePositiveIntegerCompactIndex.<br> freeStart := freeStart + numBytes.<br> newLargeInteger := newObj.<br> l6: "end allocateSmallNewSpaceSlots:format:classIndex:".<br> self cppIf: SPURVM ifTrue: [<br> self long32At: newLargeInteger + self baseHeaderSize + 0 << 2 put: (objectMemory byteSwapped32IfBigEndian: value).<br> self long32At: newLargeInteger + self baseHeaderSize + 1 << 2 put: 0<br> ] ifFalse: [self long32At: newLargeInteger + self baseHeaderSize + 0 << 2 put: (objectMemory byteSwapped32IfBigEndian: value)].<br> newLargeInteger.<br> l4: "end maybeInlinePositive32BitIntegerFor:".<br> goto l5<br> ].<br> l5: "end positive32BitIntegerFor:"<br> ].<br> stackPages longAt: sp := stackPointer - objectMemory wordSize put: object.<br> stackPointer := sp.<br><br></div><div>What sounds strange is that previous instance of that message was really behind when entering pass 6:<br><br> "begin push:".<br> object := self positiveMachineIntegerFor: vmCallbackContext thunkp asUnsignedInteger.<br> stackPages longAt: sp := stackPointer - objectMemory wordSize put: object.<br> stackPointer := sp.<br><br></div><div> It sounds like very tricky circumstances.<br>My finding is that this is due to a global state mutation (Tmethod isComplete)<br>This is used in <br><br>inlineableSend: aNode in: aCodeGen<br> "Answer true if the given send node is a call to a method that can be inlined."<br><br> | m |<br> aCodeGen maybeBreakForTestToInline: aNode in: self.<br> aNode isSend ifFalse: [ ^false ].<br> m := aCodeGen methodNamed: aNode selector. "nil if builtin or external function"<br> ^m ~= nil and: [m ~~ self and: [m isComplete and: [aCodeGen mayInline: m selector]]]<br><br></div><div>Now, thanks to this global state, we inline more than the very restrictive rules, but unfortunately sometimes wrongly...<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-04-27 21:47 GMT+02:00 Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div>Hi,<br></div><div>I changed the compiler flag -Wno-unused-value to -Wunused-value because there's no reason to carry dead code. Dead code should ring a bell.<br></div>it seems it uncovers a problem in code generation.<br></div>An example is in code generated for sendInvokeCallbackContext:<br><br> if ((argumentCountOfMethodHeader(methodHeaderOf(GIV(newMethod)))) == 4) {<br> /* begin push: */<br> value = ((usqInt)((vmCallbackContext->thunkp)));<br> /* begin positive32BitIntegerFor: */<br> /* begin maybeInlinePositive32BitIntegerFor: */<br> assert(!((hasSixtyFourBitImmediates())));<br> if ((((unsigned int) value)) <= (MaxSmallInteger)) {<br> ((value << 1) | 1);<br></div><div> ^~~~~~~~~~~~~~ PROBLEM HERE: this result should be stored in variable 'object'<br></div><div> goto l4;<br> }<br> /* begin eeInstantiateSmallClassIndex:format:numSlots: */<br></div>...snip...<br><div> object = newLargeInteger;<br> l4: /* end maybeInlinePositive32BitIntegerFor: */;<br> goto l5;<br><br> l5: /* end positive32BitIntegerFor: */;<br><br> longAtput((sp = GIV(stackPointer) - BytesPerWord), object);<br> <br></div><div>The corresponding Slang code is:<br><br> (self argumentCountOf: newMethod) = 4 ifTrue:<br> [self push: (self positiveMachineIntegerFor: vmCallbackContext thunkp asUnsignedInteger).<br><br></div><div>With push:<br><br> | sp |<br> <inline: true><br> <var: #sp type: #'char *'><br> stackPages longAt: (sp := stackPointer - objectMemory wordSize) put: object.<br> stackPointer := sp<br><br></div><div>With positiveMachineIntegerFor:<br><br> <var: #value type: #'unsigned long'><br> <inline: true><br> ^objectMemory wordSize = 8<br> ifTrue: [self positive64BitIntegerFor: value]<br> ifFalse: [self positive32BitIntegerFor: value]<br></div><div><br></div><div>With positive32BitIntegerFor:<br><br> <inline: true><br> <var: 'integerValue' type: #'unsigned int'><br> objectMemory hasSixtyFourBitImmediates<br> ifTrue:<br> [^objectMemory integerObjectOf: (integerValue asUnsignedLong bitAnd: 16rFFFFFFFF)]<br> ifFalse:<br> [^self maybeInlinePositive32BitIntegerFor: integerValue]<br><br></div><div>With maybeInlinePositive32BitIntegerFor:<br><br> <notOption: #Spur64BitMemoryManager><br> <var: 'integerValue' type: #'unsigned int'><br> | newLargeInteger |<br> self deny: objectMemory hasSixtyFourBitImmediates.<br> "force coercion because slang inliner sometimes incorrectly pass a signed int without converting to unsigned"<br> (self cCode: [self cCoerceSimple: integerValue to: #'unsigned int']<br> inSmalltalk: [integerValue bitAnd: 1 << 32 - 1]) <= objectMemory maxSmallInteger ifTrue:<br> [^objectMemory integerObjectOf: integerValue].<br> newLargeInteger := objectMemory<br> eeInstantiateSmallClassIndex: <br></div><div> ...snip...<br></div><div> ^newLargeInteger<br><br></div></div>
</blockquote></div><br></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div 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>