On Thu, Oct 16, 2014 at 11:53 PM, Marcus Denker <[email protected]> wrote:
> On 16 Oct 2014 at 22:07:03, Nicolai Hess ([email protected]) wrote: > > 2014-10-16 17:40 GMT+02:00 Marcus Denker <[email protected]>: > >> >> >> >> While in the write we need to mark: >> >> >> >> lookupVariableForWrite: aVariableNode >> >> >> >> | var | >> >> >> >> var := scope lookupVar: aVariableNode name. >> >> >> >> var ifNil: [^var]. >> >> var isSpecialVariable ifTrue: [ self storeIntoSpecialVariable: >> aVariableNode ]. >> >> var isWritable ifFalse: [ self storeIntoReadOnlyVariable: >> aVariableNode ]. >> >> >> >> var isTemp ifTrue: [ >> >> "when in a loop, all writes escape" >> >> scope outerScope isInsideOptimizedLoop ifTrue: [ var >> markEscapingWrite ]. >> >> "else only escaping when they will end up in different >> closures" >> >> (var scope outerNotOptimizedScope ~= scope >> outerNotOptimizedScope) >> >> ifTrue: [ var markEscapingWrite]]. >> >> ^var >> >> >> >> >> >> I checked that all Opal tests that are failing are actually testing >> wrong, and I double checked >> >> that all those methods are now compiled like with the old old compiler… >> >> Now recompilng the image. >> > >> > Hmm… nope…. something wrong. >> > >> >> the “outerScope” was wrong. >> >> So without that, I can recompile the image and your example is compiled >> without temp vectors… >> >> Marcus >> > > good! > I'll open an issue on fogbugz and create a slice with your changes. > > about reordering of tempvars: > It would be nice if the order preserves, but I think we can live with it. > > Yes, we should order them. But keep in mind that this does not mean that > you > > can access them via “at: index”. The index is “virtual”, so var3 can be > actually var2, > > because the *real* var2 lives together with var1 in a tempVector (which is > now var1). > > Knowledge has to come from somewhere… e.g. the DebuggerMethodMap builds up > > a description, while we (for now) just delegate to the AST (+scopes + > semantic vars). > > this means that > > #tempAt: > > sees always the real world. e.g. a method with 3 vars where they are all > in the tempVector, > > tempAt:2 will raise an error. > > namedTempAt: does what the old one did. > Not quite. What namedTempAt:[put:] do is access the temps in the order they appear in the list answered by ContextPart>>tempNames which is self debuggerMap tempNamesForContext: self. This is a flattened list that includes both direct and indirect temps. This is also the list that the Squeak debugger uses to populate the bottom-right context inspector in the debugger. namedTempAt:[put:] matches tempAt:[put:] until you get to an indirect temp. In a method the Squeak compiler always puts the indirect temps after all the direct temps, so the two match until the first indirect temp. But in a closure the situation is potentially much more complex because indirection vectors can be part of a closure's copied values, so it may have any number of indirection vectors copied form outer scopes, its own local direct temps and its own local indirect temps. The copied values get pushed after the arguments and before the local temps. So the ordering potentially gets complicated. This now can get confused in the Opal case with the changing order, which > we should fix. > > (and yes, we should check how much slower the AST bases scheme is for > accessing > > temps by offset... we could cache a representation similar to what the old > DebuggerMethodMap > > does). > Or use DebuggerMethodMap, rewriting the code to make it more comprehensible... Why reinvent the wheel? > > Although there is an issue with the debugger: > 14058 <https://pharo.fogbugz.com/default.asp?14058> Inconsistent > information in debugger > I solved that one by using tempvar names instead if the index, but > this has another issue (sometimes the last (or most nested) tempvar is > always nil). > > The real error is, EyeDebuggerContextInspector does recognizes if the > index changes > (somewhere in EyeDebuggerContextInspector>>#updateList it > compares the new elements (with new indices) with the old elements list > and skips the update) > > Ok… that could be fixed with ordered temp names? > > Marcus > > > > -- best, Eliot
