Hi Marcel,

> On Mar 26, 2020, at 5:15 AM, Marcel Taeumel <marcel.taeumel at hpi.de> wrote:
> 
> Hi Eliot, hi all!
> 
> I am having issues with the inlining of #to:do: in combination with decompiling a block and accessing the temps.
> 
> Here is a piece of code that works as expected:
> 
> | result context |
> result := OrderedCollection new.
> 
> (1 to: 5) do: [:each |
> 	result add: [:object | object + each]].	
> 
> context := (result at: 3) outerContext.
> context namedTempAt: (context tempNames indexOf: 'each'). " == 3"
> 
> HOWEVER, using #to:do: the result changes because code gets inlined:
> 
> | result context |
> result := OrderedCollection new.
> 
> 1 to: 5 do: [:each |
> 	result add: [:object | object + each]].	
> 
> context := (result at: 3) outerContext.
> context namedTempAt: (context tempNames indexOf: 'each'). " == 6"
> 
> What is my goal here? I want to unpack the block statement and fill in the temps. For all those [:object | object + each] in "result" I want to collect the strings:
> 
> -> 'object + 1'
> -> 'object + 2'
> -> 'object + 3'
> -> 'object + 4'
> -> 'object + 5'
> 
> Is there a way to access the values of temps when the block closures are created this way?

Nice one.  So the thing to realize is that, because of inlining, the second example is in fact equivalent to

| result context each |
result := OrderedCollection new.

each := 1.
[ each <= 5 ] whileTrue:
    [ result add: [:object | object + each]].

context := (result at: 3) outerContext.
context namedTempAt: (context tempNames indexOf: 'each') "= 6"

and when written like this it is clear that the search for the value will end up being as found, 6.

So then the thing to realize is that the inlining decision is wrong.  That in fact the closing over of each (the index variable of the inlined to:do:) within an uninlined inner block means that the outer block cannot be inlined without breaking the inner block.

So the next thing to determine is whether the compiler can easily be modified to identify this case and hence prevent inlining in case like these.

After that we have to assess and find out how many cases in the system (eg in trunk) would no longer be inlined and decide whether the cure is worse than the disease.  The issue here is two fold, the loss of performance due to not inlining, and the slow down to compilation due to the check(s) needed to prevent inlining in this case.

Why would adding the check potentially cause many to:[by:]do: invocations to no longer be inlined? The check is to find anywhere we see a (non-optimized) block created that closes over an index variable and is passed as an argument.  Because this is a dynamic language we cannot assume anything about the message the block is an argument to; the programmer could redefine an implementation of a method with that selector to capture the block argument just as your example does.  So we have to assume the block could be captured, and hence that the to:[by:]do: in which it occurs cannot be inlined.

Other Smalltalks have decided to live with the disease.  I think that the right decision is *not* to live with the disease, but to fix the inliner, and wait for Scorch to “do the right thing” and inline dynamically.  Scorch can inline precisely because it does indeed find out about the caller, and inline if the caller does not capture the block, and adds the dependency information to enable the optimized version to be discarded if redefinition occurs.

The first thing to do is write a test asserting the correct behaviour.

The second thing to do is learn how and where the bytecode compiler decides whether or not to inline to:[by:]do:.

The third thing to do is to modify that inlining decision to generate correct code for your case.

The fourth thing to do is then to find out how many methods are affected.

Finally we can then assess whether we want to live with the disease or not.

> Best,
> Marcel

Best, Eliot
_,,,^..^,,,_ (phone)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20200326/12403242/attachment-0001.html>