Hi David,<br><br><div class="gmail_quote">On Tue, Apr 2, 2013 at 7:46 PM, David T. Lewis <span dir="ltr"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:<br>
> On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:<br>
> > On 29 March 2013 14:17, David T. Lewis <<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>> wrote:<br>
> > ><br>
> > > I moved these two new tests from Inbox to Trunk. One test fails on recent<br>
> > > trunk images. I do not know if this is a bug or a feature, but I cannot<br>
> > > explain the regression so I don't want to assume it's a feature.<br>
> > ><br>
> > > Dave<br>
> ><br>
> > <a href="http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/CompilerTest/testMaxLiteralsWithClassReferenceInClosure/" target="_blank">http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/CompilerTest/testMaxLiteralsWithClassReferenceInClosure/</a><br>
> ><br>
> > shows the test failure.<br>
><br>
> The actual change to the image that causes this test failure occurred between<br>
> SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had been<br>
> part of the test suite all along, then you would have seen the failure appear<br>
> in 213.<br>
><br>
> I suspect that this is a side effect of class lookups associated with the Environments<br>
> changes, but I cannot say if it is a bug or an explainable variation. Either way<br>
> it's good to note the change, and we can either fix the bug or fix the test<br>
> as needed.<br>
<br>
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]<br>
<br>
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect<br>
is somehow related to the apparently incorrect warning about too many literals.<br></blockquote><div><br></div><div>I doubt it. How to reproduce the incorrect warning? The point about LiteralVariableNode>>sizeCodeForStore: (& LiteralVariableNode>>emitCodeForStore:encoder:) is that they don't do what's advertised. i.e. they don't leave the value of the expression that is stored on top of stack unless the special write binding answers the expression in response to #value:. So if one were to write</div>
<div><br></div><div> v := SomeSpecialBinding := expr</div><div><br></div><div>v would end up holding SomeSpecialBinding, not expr.</div><div><br></div><div>This could perhaps be corrected by ensuring that all classes that implement isSpecialWriteBinding also implement #value: to answer the argument, not the receiver. e.g. instead of</div>
<div><br></div><div>Binding methods for accessing</div><div><div>value: anObject</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>(AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings')</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>ifTrue: [value := anObject]</div></div><div><br></div><div>we use</div><div><br></div><div><div>value: anObject</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>(AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings')</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>ifTrue: [value := anObject].</div></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>^anObject</div><div><br></div><div>but that's tricky.</div>
<div><br></div><div>The problem for the code generator is that there's no swap operation. e.g. the following would work:</div><div><br></div><div>stack before:</div><div> expr</div><div><br></div><div>first half of code:</div>
<div> dup</div><div> push binding</div><div> swap</div><div><br></div><div>stack during:</div><div> expr</div><div> binding</div><div> expr</div><div><br></div><div>second half of code</div><div> send #value:</div>
<div> pop</div><div><br></div><div>stack after</div><div> expr</div><div><br></div><div>but that's a) a new bytecode and b) a lot of bytes to do a store.</div><div><br></div><div>A different approach would be to rewrite emit because it knows the stack pointer to write something like</div>
<div> push binding</div><div> push temp N (where N is the offset of expr on the stack)</div><div> send value:</div><div><br></div><div>I think this is the correct approach. It might require fixing the decompiler, but it shouldn't be hard. I'll have a go. In any case we need a test that ensures that</div>
<div><br></div><div> | v expr |</div><div> expr := Object new.</div><div> v := SomeSpecialBinding := expr.</div><div> self assert: v == expr</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Reducing to a smaller test case, compiling 'Time now' yields a parse tree<br>
with LiteralVariableNode {Time} that has code -4 which causes dual reserving<br>
in sizeCodeForValue: and this in turn leads to a mis-counting of literals<br>
in the encoder.<br>
<br>
In an earlier version of trunk, the same LiteralVariableNode has a non-negative<br>
code value that does not produce the dual reserving in sizeCodeForValue:<br>
<br>
Dave<br>
<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>