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

Register aliasing in load_traceinput raises an exception #1449

Open
ltratt opened this issue Oct 31, 2024 · 1 comment
Open

Register aliasing in load_traceinput raises an exception #1449

ltratt opened this issue Oct 31, 2024 · 1 comment
Assignees

Comments

@ltratt
Copy link
Contributor

ltratt commented Oct 31, 2024

For boring reasons I changed yklua's vmfetch thus:

diff --git src/lvm.c src/lvm.c
index f796a756..aa351a0b 100644
--- src/lvm.c
+++ src/lvm.c
@@ -1139,7 +1139,7 @@ void luaV_finishOp (lua_State *L) {
     trap = luaG_traceexec(L, pc);  /* handle hooks */ \
     updatebase(ci);  /* correct stack */ \
   } \
-  i = (Instruction) yk_promote(*(pc++)); \
+  i = (Instruction) yk_promote((uintptr_t) *(pc++)); \
 }
 #else
 #  define vmfetch()	{ \

This then causes the resulting trace to alias registers: one register (I think) has a the uintptr_t value in it (8 bytes) and the other the Instruction value (4 bytes). The start of the resulting trace thus looks as follows:

    %45: i64 = load_ti Register(0, 8, 0, [])
    %46: i32 = load_ti Register(0, 4, 0, [])
    %47: i32 = load_ti Register(3, 4, 0, [])
    %48: ptr = load_ti Register(4, 8, 0, [])
    %49: ptr = load_ti Register(5, 8, 0, [])

Notice that %45 and %46 both reference register 0. This then causes our register allocator to explode as it expects a register to reference a single value.

I'm not sure that fixing this is very high priority, but it does show that LLVM can generate a style of IR that we haven't considered before. It might be worth keeping this in the back of our mind.

@ltratt ltratt assigned vext01 and unassigned ptersilie Nov 21, 2024
@ltratt
Copy link
Contributor Author

ltratt commented Nov 21, 2024

We have now seen this happen again, and happen more often.

ltratt added a commit to ltratt/yk that referenced this issue Nov 21, 2024
Currently our register allocator assumes that SSA variables can't alias.
This is true for the parts under our control -- but isn't true of the
inputs to a trace! For example both `%0` and `%1` might come from `RAX`.
This caused a panic in the register allocator.

I'd noted the problem in ykjit#1449 but
thought it happened rarely. Edd noticed that it happens in other, more
common situations (including as a result of common subexpression
elimination), including in a yklua benchmark, and created a test for it.

This commit is a "quick fix" (it's not pretty, or efficient): when we
spot aliasing, we spill aliased variables onto the stack, implicitly
dealiasing them. In other words, we avoid having to think about aliasing
in the main bulk of the register allocator at the expense of having to
spill (and potentially unspill) values unnecessarily.

Co-authored-by: Edd Barrett <[email protected]>
ltratt added a commit to ltratt/yk that referenced this issue Nov 21, 2024
Currently our register allocator assumes that SSA variables can't alias.
This is true for the parts under our control -- but isn't true of the
inputs to a trace! For example both `%0` and `%1` might come from `RAX`.
This caused a panic in the register allocator.

I'd noted the problem in ykjit#1449 but
thought it happened rarely. Edd noticed that it happens in other, more
common situations (including as a result of common subexpression
elimination), including in a yklua benchmark, and created a test for it.

This commit is a "quick fix" (it's not pretty, or efficient): when we
spot aliasing, we spill aliased variables onto the stack, implicitly
dealiasing them. In other words, we avoid having to think about aliasing
in the main bulk of the register allocator at the expense of having to
spill (and potentially unspill) values unnecessarily.

Co-authored-by: Edd Barrett <[email protected]>
vext01 added a commit to vext01/yk_pv that referenced this issue Nov 26, 2024
Currently our register allocator assumes that SSA variables can't alias.
This is true for the parts under our control -- but isn't true of the
inputs to a trace! For example both `%0` and `%1` might come from `RAX`.
This caused a panic in the register allocator.

I'd noted the problem in ykjit#1449 but
thought it happened rarely. Edd noticed that it happens in other, more
common situations (including as a result of common subexpression
elimination), including in a yklua benchmark, and created a test for it.

This commit is a "quick fix" (it's not pretty, or efficient): when we
spot aliasing, we spill aliased variables onto the stack, implicitly
dealiasing them. In other words, we avoid having to think about aliasing
in the main bulk of the register allocator at the expense of having to
spill (and potentially unspill) values unnecessarily.

Co-authored-by: Edd Barrett <[email protected]>
vext01 added a commit to vext01/yk_pv that referenced this issue Nov 26, 2024
Currently our register allocator assumes that SSA variables can't alias.
This is true for the parts under our control -- but isn't true of the
inputs to a trace! For example both `%0` and `%1` might come from `RAX`.
This caused a panic in the register allocator.

I'd noted the problem in ykjit#1449 but
thought it happened rarely. Edd noticed that it happens in other, more
common situations (including as a result of common subexpression
elimination), including in a yklua benchmark, and created a test for it.

This commit is a "quick fix" (it's not pretty, or efficient): when we
spot aliasing, we spill aliased variables onto the stack, implicitly
dealiasing them. In other words, we avoid having to think about aliasing
in the main bulk of the register allocator at the expense of having to
spill (and potentially unspill) values unnecessarily.

Co-authored-by: Edd Barrett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants