| Message ID | 1437755447-10537-5-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 07/24/2015 09:30 AM, Aurelien Jarno wrote: > Now that copies and constants are tracked separately, we can allow > constant to have copies, deferring the choice to use a register or a > constant to the register allocation pass. This prevent this kind of > regular constant reloading: This appears to cause $ gdb --args ./x86_64-linux-user/qemu-x86_64 /bin/ls ... (gdb) run Starting program: /home/rth/work/qemu/bld/x86_64-linux-user/qemu-x86_64 /bin/ls [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7ffff6604700 (LWP 7767)] qemu-x86_64: /home/rth/work/qemu/qemu/tcg/tcg.c:1827: tcg_reg_alloc_bb_end: Assertion `ts->val_type == TEMP_VAL_DEAD' failed. Program received signal SIGABRT, Aborted. r~
On 2015-07-24 13:15, Richard Henderson wrote: > On 07/24/2015 09:30 AM, Aurelien Jarno wrote: > > Now that copies and constants are tracked separately, we can allow > > constant to have copies, deferring the choice to use a register or a > > constant to the register allocation pass. This prevent this kind of > > regular constant reloading: > > This appears to cause > > $ gdb --args ./x86_64-linux-user/qemu-x86_64 /bin/ls > ... > (gdb) run > Starting program: /home/rth/work/qemu/bld/x86_64-linux-user/qemu-x86_64 /bin/ls > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib64/libthread_db.so.1". > [New Thread 0x7ffff6604700 (LWP 7767)] > qemu-x86_64: /home/rth/work/qemu/qemu/tcg/tcg.c:1827: tcg_reg_alloc_bb_end: > Assertion `ts->val_type == TEMP_VAL_DEAD' failed. > > Program received signal SIGABRT, Aborted. It looks like I have forgotten to configure with --enable-debug-tcg. With it I am able to reproduce the problem. It seems to be a bug in the liveness analysis or the register allocator, I will try to come with a patch soon.
Aurelien Jarno <aurelien@aurel32.net> writes: > Now that copies and constants are tracked separately, we can allow > constant to have copies, deferring the choice to use a register or a > constant to the register allocation pass. This prevent this kind of > regular constant reloading: > > -OUT: [size=338] > +OUT: [size=298] > mov -0x4(%r14),%ebp > test %ebp,%ebp > jne 0x7ffbe9cb0ed6 > mov $0x40002219f8,%rbp > mov %rbp,(%r14) > - mov $0x40002219f8,%rbp > mov $0x4000221a20,%rbx > mov %rbp,(%rbx) > mov $0x4000000000,%rbp > mov %rbp,(%r14) > - mov $0x4000000000,%rbp > mov $0x4000221d38,%rbx > mov %rbp,(%rbx) > mov $0x40002221a8,%rbp > mov %rbp,(%r14) > - mov $0x40002221a8,%rbp > mov $0x4000221d40,%rbx > mov %rbp,(%rbx) > mov $0x4000019170,%rbp > mov %rbp,(%r14) > - mov $0x4000019170,%rbp > mov $0x4000221d48,%rbx > mov %rbp,(%rbx) > mov $0x40000049ee,%rbp > mov %rbp,0x80(%r14) > mov %r14,%rdi > callq 0x7ffbe99924d0 > mov $0x4000001680,%rbp > mov %rbp,0x30(%r14) > mov 0x10(%r14),%rbp > mov $0x4000001680,%rbp > mov %rbp,0x30(%r14) > mov 0x10(%r14),%rbp > shl $0x20,%rbp > mov (%r14),%rbx > mov %ebx,%ebx > mov %rbx,(%r14) > or %rbx,%rbp > mov %rbp,0x10(%r14) > mov %rbp,0x90(%r14) > mov 0x60(%r14),%rbx > mov %rbx,0x38(%r14) > mov 0x28(%r14),%rbx > mov $0x4000220e60,%r12 > mov %rbx,(%r12) > mov $0x40002219c8,%rbx > mov %rbp,(%rbx) > mov 0x20(%r14),%rbp > sub $0x8,%rbp > mov $0x4000004a16,%rbx > mov %rbx,0x0(%rbp) > mov %rbp,0x20(%r14) > mov $0x19,%ebp > mov %ebp,0xa8(%r14) > mov $0x4000015110,%rbp > mov %rbp,0x80(%r14) > xor %eax,%eax > jmpq 0x7ffbebcae426 > lea -0x5f6d72a(%rip),%rax # 0x7ffbe3d437b3 > jmpq 0x7ffbebcae426 > > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > --- > tcg/optimize.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/tcg/optimize.c b/tcg/optimize.c > index f16eb1e..48103b2 100644 > --- a/tcg/optimize.c > +++ b/tcg/optimize.c > @@ -237,11 +237,6 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > return; > } > > - if (temp_is_const(src)) { > - tcg_opt_gen_movi(s, op, args, dst, temps[src].val); > - return; > - } > - That looks suspicious, surely we only want to drop the move if we already have the const somewhere else? > TCGOpcode new_op = op_to_mov(op->opc); > tcg_target_ulong mask; > > @@ -255,17 +250,15 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > } > temps[dst].mask = mask; > > - assert(!temp_is_const(src)); > - > if (s->temps[src].type == s->temps[dst].type) { > if (!temp_is_copy(src)) { > - temps[src].is_const = false; > temps[src].is_copy = true; > temps[src].next_copy = src; > temps[src].prev_copy = src; > } > - temps[dst].is_const = false; > + temps[dst].is_const = temps[src].is_const; > temps[dst].is_copy = true; > + temps[dst].val = temps[src].val; > temps[dst].next_copy = temps[src].next_copy; > temps[dst].prev_copy = src; > temps[temps[dst].next_copy].prev_copy = dst;
On 2015-07-29 17:12, Alex Bennée wrote: > > Aurelien Jarno <aurelien@aurel32.net> writes: > > > Now that copies and constants are tracked separately, we can allow > > constant to have copies, deferring the choice to use a register or a > > constant to the register allocation pass. This prevent this kind of > > regular constant reloading: > > > > -OUT: [size=338] > > +OUT: [size=298] > > mov -0x4(%r14),%ebp > > test %ebp,%ebp > > jne 0x7ffbe9cb0ed6 > > mov $0x40002219f8,%rbp > > mov %rbp,(%r14) > > - mov $0x40002219f8,%rbp > > mov $0x4000221a20,%rbx > > mov %rbp,(%rbx) > > mov $0x4000000000,%rbp > > mov %rbp,(%r14) > > - mov $0x4000000000,%rbp > > mov $0x4000221d38,%rbx > > mov %rbp,(%rbx) > > mov $0x40002221a8,%rbp > > mov %rbp,(%r14) > > - mov $0x40002221a8,%rbp > > mov $0x4000221d40,%rbx > > mov %rbp,(%rbx) > > mov $0x4000019170,%rbp > > mov %rbp,(%r14) > > - mov $0x4000019170,%rbp > > mov $0x4000221d48,%rbx > > mov %rbp,(%rbx) > > mov $0x40000049ee,%rbp > > mov %rbp,0x80(%r14) > > mov %r14,%rdi > > callq 0x7ffbe99924d0 > > mov $0x4000001680,%rbp > > mov %rbp,0x30(%r14) > > mov 0x10(%r14),%rbp > > mov $0x4000001680,%rbp > > mov %rbp,0x30(%r14) > > mov 0x10(%r14),%rbp > > shl $0x20,%rbp > > mov (%r14),%rbx > > mov %ebx,%ebx > > mov %rbx,(%r14) > > or %rbx,%rbp > > mov %rbp,0x10(%r14) > > mov %rbp,0x90(%r14) > > mov 0x60(%r14),%rbx > > mov %rbx,0x38(%r14) > > mov 0x28(%r14),%rbx > > mov $0x4000220e60,%r12 > > mov %rbx,(%r12) > > mov $0x40002219c8,%rbx > > mov %rbp,(%rbx) > > mov 0x20(%r14),%rbp > > sub $0x8,%rbp > > mov $0x4000004a16,%rbx > > mov %rbx,0x0(%rbp) > > mov %rbp,0x20(%r14) > > mov $0x19,%ebp > > mov %ebp,0xa8(%r14) > > mov $0x4000015110,%rbp > > mov %rbp,0x80(%r14) > > xor %eax,%eax > > jmpq 0x7ffbebcae426 > > lea -0x5f6d72a(%rip),%rax # 0x7ffbe3d437b3 > > jmpq 0x7ffbebcae426 > > > > Cc: Richard Henderson <rth@twiddle.net> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > > --- > > tcg/optimize.c | 11 ++--------- > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > diff --git a/tcg/optimize.c b/tcg/optimize.c > > index f16eb1e..48103b2 100644 > > --- a/tcg/optimize.c > > +++ b/tcg/optimize.c > > @@ -237,11 +237,6 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > > return; > > } > > > > - if (temp_is_const(src)) { > > - tcg_opt_gen_movi(s, op, args, dst, temps[src].val); > > - return; > > - } > > - > > That looks suspicious, surely we only want to drop the move if we > already have the const somewhere else? That's actually the while point of this patchset, to avoid converting mov into moving for constant values and thus loosing the link between temps. At this moment point of the code, the only way to know that the source temp is a constant is when it has been set using a movi before.
Aurelien Jarno <aurelien@aurel32.net> writes: > On 2015-07-29 17:12, Alex Bennée wrote: >> >> Aurelien Jarno <aurelien@aurel32.net> writes: >> >> > Now that copies and constants are tracked separately, we can allow >> > constant to have copies, deferring the choice to use a register or a >> > constant to the register allocation pass. This prevent this kind of >> > regular constant reloading: >> > >> > -OUT: [size=338] >> > +OUT: [size=298] >> > mov -0x4(%r14),%ebp >> > test %ebp,%ebp >> > jne 0x7ffbe9cb0ed6 >> > mov $0x40002219f8,%rbp >> > mov %rbp,(%r14) >> > - mov $0x40002219f8,%rbp >> > mov $0x4000221a20,%rbx >> > mov %rbp,(%rbx) >> > mov $0x4000000000,%rbp >> > mov %rbp,(%r14) <snip> >> > --- a/tcg/optimize.c >> > +++ b/tcg/optimize.c >> > @@ -237,11 +237,6 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, >> > return; >> > } >> > >> > - if (temp_is_const(src)) { >> > - tcg_opt_gen_movi(s, op, args, dst, temps[src].val); >> > - return; >> > - } >> > - >> >> That looks suspicious, surely we only want to drop the move if we >> already have the const somewhere else? > > That's actually the while point of this patchset, to avoid converting > mov into moving for constant values and thus loosing the link between > temps. I get that, I guess I didn't follow the previous loading of the constant value. Maybe showing the ops in the commit message will make it clearer. > > At this moment point of the code, the only way to know that the source > temp is a constant is when it has been set using a movi before. OK.
On 2015-07-29 21:42, Alex Bennée wrote: > > Aurelien Jarno <aurelien@aurel32.net> writes: > > > On 2015-07-29 17:12, Alex Bennée wrote: > >> > >> Aurelien Jarno <aurelien@aurel32.net> writes: > >> > >> > Now that copies and constants are tracked separately, we can allow > >> > constant to have copies, deferring the choice to use a register or a > >> > constant to the register allocation pass. This prevent this kind of > >> > regular constant reloading: > >> > > >> > -OUT: [size=338] > >> > +OUT: [size=298] > >> > mov -0x4(%r14),%ebp > >> > test %ebp,%ebp > >> > jne 0x7ffbe9cb0ed6 > >> > mov $0x40002219f8,%rbp > >> > mov %rbp,(%r14) > >> > - mov $0x40002219f8,%rbp > >> > mov $0x4000221a20,%rbx > >> > mov %rbp,(%rbx) > >> > mov $0x4000000000,%rbp > >> > mov %rbp,(%r14) > <snip> > >> > --- a/tcg/optimize.c > >> > +++ b/tcg/optimize.c > >> > @@ -237,11 +237,6 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > >> > return; > >> > } > >> > > >> > - if (temp_is_const(src)) { > >> > - tcg_opt_gen_movi(s, op, args, dst, temps[src].val); > >> > - return; > >> > - } > >> > - > >> > >> That looks suspicious, surely we only want to drop the move if we > >> already have the const somewhere else? > > > > That's actually the while point of this patchset, to avoid converting > > mov into moving for constant values and thus loosing the link between > > temps. > > I get that, I guess I didn't follow the previous loading of the constant > value. Maybe showing the ops in the commit message will make it clearer. Ok, I'll do that in v3.
diff --git a/tcg/optimize.c b/tcg/optimize.c index f16eb1e..48103b2 100644 --- a/tcg/optimize.c +++ b/tcg/optimize.c @@ -237,11 +237,6 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, return; } - if (temp_is_const(src)) { - tcg_opt_gen_movi(s, op, args, dst, temps[src].val); - return; - } - TCGOpcode new_op = op_to_mov(op->opc); tcg_target_ulong mask; @@ -255,17 +250,15 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, } temps[dst].mask = mask; - assert(!temp_is_const(src)); - if (s->temps[src].type == s->temps[dst].type) { if (!temp_is_copy(src)) { - temps[src].is_const = false; temps[src].is_copy = true; temps[src].next_copy = src; temps[src].prev_copy = src; } - temps[dst].is_const = false; + temps[dst].is_const = temps[src].is_const; temps[dst].is_copy = true; + temps[dst].val = temps[src].val; temps[dst].next_copy = temps[src].next_copy; temps[dst].prev_copy = src; temps[temps[dst].next_copy].prev_copy = dst;
Now that copies and constants are tracked separately, we can allow constant to have copies, deferring the choice to use a register or a constant to the register allocation pass. This prevent this kind of regular constant reloading: -OUT: [size=338] +OUT: [size=298] mov -0x4(%r14),%ebp test %ebp,%ebp jne 0x7ffbe9cb0ed6 mov $0x40002219f8,%rbp mov %rbp,(%r14) - mov $0x40002219f8,%rbp mov $0x4000221a20,%rbx mov %rbp,(%rbx) mov $0x4000000000,%rbp mov %rbp,(%r14) - mov $0x4000000000,%rbp mov $0x4000221d38,%rbx mov %rbp,(%rbx) mov $0x40002221a8,%rbp mov %rbp,(%r14) - mov $0x40002221a8,%rbp mov $0x4000221d40,%rbx mov %rbp,(%rbx) mov $0x4000019170,%rbp mov %rbp,(%r14) - mov $0x4000019170,%rbp mov $0x4000221d48,%rbx mov %rbp,(%rbx) mov $0x40000049ee,%rbp mov %rbp,0x80(%r14) mov %r14,%rdi callq 0x7ffbe99924d0 mov $0x4000001680,%rbp mov %rbp,0x30(%r14) mov 0x10(%r14),%rbp mov $0x4000001680,%rbp mov %rbp,0x30(%r14) mov 0x10(%r14),%rbp shl $0x20,%rbp mov (%r14),%rbx mov %ebx,%ebx mov %rbx,(%r14) or %rbx,%rbp mov %rbp,0x10(%r14) mov %rbp,0x90(%r14) mov 0x60(%r14),%rbx mov %rbx,0x38(%r14) mov 0x28(%r14),%rbx mov $0x4000220e60,%r12 mov %rbx,(%r12) mov $0x40002219c8,%rbx mov %rbp,(%rbx) mov 0x20(%r14),%rbp sub $0x8,%rbp mov $0x4000004a16,%rbx mov %rbx,0x0(%rbp) mov %rbp,0x20(%r14) mov $0x19,%ebp mov %ebp,0xa8(%r14) mov $0x4000015110,%rbp mov %rbp,0x80(%r14) xor %eax,%eax jmpq 0x7ffbebcae426 lea -0x5f6d72a(%rip),%rax # 0x7ffbe3d437b3 jmpq 0x7ffbebcae426 Cc: Richard Henderson <rth@twiddle.net> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- tcg/optimize.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)