Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stores to dead, unused objects prevent allocation sinking #651

Open
alexshpilkin opened this issue Dec 29, 2020 · 0 comments · May be fixed by #652
Open

Stores to dead, unused objects prevent allocation sinking #651

alexshpilkin opened this issue Dec 29, 2020 · 0 comments · May be fixed by #652

Comments

@alexshpilkin
Copy link

Consider the following example:

local ffi = require "ffi"
local point = ffi.typeof[[ struct { double x, y; } ]]

local v = point(1.0, 2.0)
for i = 1, 100000000 do
	--[[A]] local t = {v}
	v = point(v.y, v.x)
end
print(v.x, v.y)

If line A is commented out, LuaJIT is (of course) able to completely eliminate allocations in the inner loop, but as it is the allocations remain and the loop is slowed down dramatically, even though the t variable is completely dead and unused—it can’t escape the loop and isn’t read inside it.

Yes, this example is silly when written out explicitly, but it’s essentially what the compiler sees when an aggregate used in a loop is written into a temporary data structure, but all references to that temporary are eliminated by store-to-load forwarding.

The issue appears as follows: (1) DCE marking sees that t is dead; (2) DCE propagation sees the store of v into t[0] and switches t to live; (4) SINK marking marks v as used because it’s stored into t[0]; (5) SINK sweep successfully sinks the allocation of t but is unable to sink the allocation of v because the previous step marked it.

It appears to me that the easiest solution would be to make DCE eliminate the store on step (2). Unfortunately, that means that DCE would need to do a form of alias analysis, even if a very weak one would be sufficient for the example (“is the object we’re storing to loaded from at all?”). Also, the DCE pass (1,2) I’m talking about isn’t even the main DCE pass—that happens in ASM and therefore after SINK—so I’m not sure how appropriate it would be to extend it this way.

Alternatively, iterating SINK (4,5) to sink values of stores to objects whose allocation has been sunk... Would cover more cases (above, copy line A outside the loop and replace the copy inside by t[0] = v) but would also make snapshots much, much more complicated, so I don’t think that’s feasible.

As you can see, this enhancement request is a bit pie-in-the-sky, so feel free to close it without mercy if you feel it’s not worth it. Otherwise I would be up to implementing it, but would probably need to hear your general thoughts on the design above, first.

@XmiliaH XmiliaH linked a pull request Jan 5, 2021 that will close this issue
@MikePall MikePall mentioned this issue Sep 21, 2023
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants