<div dir="ltr"><div><div><div><div>Hi Eliot, I'll check tonight.<br><br></div></div>My concern was that isComplete state is bound to TMethod.<br>But once isComplete it seems to have effect on inlining of each send.<br></div>If inlining is decomposed as happens for one of the message sends, the assignment is well distributed in each ifTrue:ifFalse: branch, but once isComplete and applied onto other message sends, it seems that the inliner does not take the same path.<br><br>It would be great to turn workspace snippets into non regression tests, or better specification tests. We need more hands to engineer this.<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-04-28 4:36 GMT+02:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br><div dir="ltr">Hi Nicolas,<br><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><div><div>It sounds like related to ifTrue:ifFalse: alternatives.<br></div></div></div></blockquote><div><br></div><div>Indeed it is. The first bug is that inlineCodeOrNilForStatement:in: is wrong to state that there is no direct return for a send node. If the send node is part of a returning if then there is a direct return. The second bug is that if one fixes this bug then exitVar:label: is too naive in mapping ^expr into exitVar := expr; got exitLabel. It uses replaceNodesIn: which is strictly top-down, and so the replacement for ^expr ifTrue: [...^fu...] ifFalse: [...^bar...] will prevent replacement of either ^fu or ^bar. I think I have the fix. I'm just testing now...</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div></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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>
<br></blockquote></div><br></div>