OK, I fixed this (a pleasant Monday afternoon distraction) (at least I *think* I've fixed it; I still have to check the decompiler). But... in doing so I realised I had to fix CompiledMethod>>#= since I needed to see exactly which methods the compiler change affected (just committed as Kernel-eem.669). In doing this I noticed that recent compiler changes have not been applied by recompiling affected methods in the system. If you look at Behavior>>#sourceMatchesBytecodeAt: it uses CompiledMethod>>#= to check that the compiler's output remains unchanged. In the 4.3 trunk image with CompiledMethod>>#= fixed, sourceMatchesBytecodeAt: reports that some 19654 methods need recompiling:<div>
<br></div><div><div><span class="Apple-tab-span" style="white-space:pre"> </span>(SystemNavigation new allSelect: [:m| (m methodClass sourceMatchesBytecodeAt: m selector) not]) size 19654</div><div><br></div><div>Being selective in recompiling, via e.g.</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>SystemNavigation new allSelect: [:m| (m methodClass sourceMatchesBytecodeAt: m selector) ifFalse: [m methodClass recompile: m selector]. false]</div>
<div><br></div><div>one gets to the desired</div><div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>(SystemNavigation new allSelect: [:m| (m methodClass sourceMatchesBytecodeAt: m selector) not]) size 0</div>
</div><div><br></div><div>So the question is should we add an update that does the selective recompile? I think we should, e.g. by committing a version of Compiler that contains the selective recompile as a post-script. Agreed?</div>
<br><div class="gmail_quote">On Mon, Feb 13, 2012 at 11:58 AM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com">eliot.miranda@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi All,<div><br></div><div> (whitewash alert) I just had occasion to look at the bytecode generated by the standard compiler for HashedCollection class>>#goodPrimeAtLeast:. Here's the source, with the issue in bold:</div>
<div><br></div><div><div>goodPrimeAtLeast: lowerLimit</div><div><span style="white-space:pre-wrap"> </span>"Answer the next good prime >= lowerlimit.</div><div><span style="white-space:pre-wrap"> </span>If lowerLimit is larger than the largest known good prime,</div>
<div><span style="white-space:pre-wrap"> </span>just make it odd."</div><div><span style="white-space:pre-wrap"> </span></div><div><span style="white-space:pre-wrap"> </span>| primes low mid high prime |</div>
<div><span style="white-space:pre-wrap"> </span>primes := self goodPrimes.</div><div><span style="white-space:pre-wrap"> </span>low := 1.</div><div><span style="white-space:pre-wrap"> </span>high := primes size.</div>
<div><span style="white-space:pre-wrap"> </span>lowerLimit > (primes at: high) ifTrue: [</div><div><span style="white-space:pre-wrap"> </span>^lowerLimit bitOr: 1 ].</div><div><span style="white-space:pre-wrap"> </span>[ high - low <= 1 ] whileFalse: [</div>
<div><span style="white-space:pre-wrap"> </span>mid := high + low // 2.</div><div><span style="white-space:pre-wrap"> </span>prime := primes at: mid.</div><div><span style="white-space:pre-wrap"> </span>prime = lowerLimit ifTrue: [ ^prime ].</div>
<div><span style="white-space:pre-wrap"> </span><b>prime < lowerLimit</b></div><div><b><span style="white-space:pre-wrap"> </span>ifTrue: [ low := mid ]</b></div><div><b><span style="white-space:pre-wrap"> </span>ifFalse: [ high := mid ]</b> ].</div>
<div><span style="white-space:pre-wrap"> </span>(primes at: low) >= lowerLimit ifTrue: [ ^primes at: low ].</div><div><span style="white-space:pre-wrap"> </span>^primes at: high</div>
<div><br></div><div>The code for this sequence is</div><div><div><span style="white-space:pre-wrap"> </span>58 <15> pushTemp: 5</div><div><span style="white-space:pre-wrap"> </span>59 <10> pushTemp: 0</div>
<div><span style="white-space:pre-wrap"> </span>60 <B2> send: <</div><div><span style="white-space:pre-wrap"> </span>61 <9B> jumpFalse: 66</div><div><span style="white-space:pre-wrap"> </span>62 <13> pushTemp: 3</div>
<div><span style="white-space:pre-wrap"> </span><b>63 <81 42> storeIntoTemp: 2</b></div><div><span style="white-space:pre-wrap"> </span>65 <92> jumpTo: 69</div><div><span style="white-space:pre-wrap"> </span>66 <13> pushTemp: 3</div>
<div><span style="white-space:pre-wrap"> </span><b>67 <81 44> storeIntoTemp: 4</b></div><div><span style="white-space:pre-wrap"> </span><b>69 <87> pop</b></div></div><div><br>
</div><div>where-as the following would be better:</div><div><br></div><div><div><span style="white-space:pre-wrap"> </span>58 <15> pushTemp: 5</div><div><span style="white-space:pre-wrap"> </span>59 <10> pushTemp: 0</div>
<div><span style="white-space:pre-wrap"> </span>60 <B2> send: <</div><div><span style="white-space:pre-wrap"> </span>61 <9B> jumpFalse: 66</div><div><span style="white-space:pre-wrap"> </span>62 <13> pushTemp: 3</div>
<div><span style="white-space:pre-wrap"> </span>63 <82 42> popIntoTemp: 2</div><div><span style="white-space:pre-wrap"> </span>65 <92> jumpTo: 69</div><div><span style="white-space:pre-wrap"> </span>66 <13> pushTemp: 3</div>
<div><span style="white-space:pre-wrap"> </span>67 <82 44> popIntoTemp: 4</div></div><div><br></div><div>The reason is that the code generator favours using a single pop for both arms of the if:</div>
<div><br></div><div>MessageNode>>sizeCodeForIf: encoder value: forValue</div><div><span style="white-space:pre-wrap"> </span>| thenExpr elseExpr branchSize thenSize elseSize |</div><div><span style="white-space:pre-wrap"> </span>thenExpr := arguments at: 1.</div>
<div><span style="white-space:pre-wrap"> </span>elseExpr := arguments at: 2.</div><div><span style="white-space:pre-wrap"> </span>(forValue</div><div><span style="white-space:pre-wrap"> </span> or: [(thenExpr isJust: NodeNil)</div>
<div><span style="white-space:pre-wrap"> </span> or: [elseExpr isJust: NodeNil]]) not</div><div><span style="white-space:pre-wrap"> </span>"(...not ifTrue: avoids using ifFalse: alone during this compile)"</div>
<div><span style="white-space:pre-wrap"> </span>ifTrue: <b>"Two-armed IFs forEffect share a single pop"</b></div><div><span style="white-space:pre-wrap"> </span>[^super sizeCodeForEffect: encoder].</div>
<div><span style="white-space:pre-wrap"> </span>...</div><div><br></div><div>MessageNode>>emitCodeForIf: stack encoder: encoder value: forValue</div><div><span style="white-space:pre-wrap"> </span>| thenExpr thenSize elseExpr elseSize |</div>
<div><span style="white-space:pre-wrap"> </span>thenSize := sizes at: 1.</div><div><span style="white-space:pre-wrap"> </span>elseSize := sizes at: 2.</div><div><span style="white-space:pre-wrap"> </span>(forValue not and: [elseSize * thenSize > 0]) ifTrue:</div>
<div><span style="white-space:pre-wrap"> </span><b>"Two-armed IFs forEffect share a single pop"</b></div><div><span style="white-space:pre-wrap"> </span>[^super emitCodeForEffect: stack encoder: encoder].</div>
<div><br></div><div>It would be nice if this only happened if doing so actually reduced the size of the generated code.</div>-- <br><font color="#888888">best,<div>Eliot</div><br>
</font></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div><br>
</div>