| Message ID | 1437780852-1549-1-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 07/24/2015 04:34 PM, Aurelien Jarno wrote: > ots->val_type = TEMP_VAL_CONST; > ots->val = ts->val; > + if (IS_DEAD_ARG(1)) { > + temp_dead(s, args[1]); > + } Aren't we also missing if (NEED_SYNC_ARG(0)) { temp_sync(s, args[0], allocated_regs); } along this path? Seems like there's room for cleanup here, at least for 2.5... r~
On 2015-07-25 15:06, Richard Henderson wrote: > On 07/24/2015 04:34 PM, Aurelien Jarno wrote: > > ots->val_type = TEMP_VAL_CONST; > > ots->val = ts->val; > >+ if (IS_DEAD_ARG(1)) { > >+ temp_dead(s, args[1]); > >+ } > > Aren't we also missing > > if (NEED_SYNC_ARG(0)) { > temp_sync(s, args[0], allocated_regs); > } > > along this path? I don't think so, I guess it's covered by the first part of this function: | if (((NEED_SYNC_ARG(0) || ots->fixed_reg) && ts->val_type != TEMP_VAL_REG) | || ts->val_type == TEMP_VAL_MEM) { It means after this block, a value that need to be synced will always be in a register, including in the constant case. > Seems like there's room for cleanup here, at least for 2.5... I agree we might want to clean a bit the code. It might even go beyond this function, as we repeatedly have almost the same code in various places of tcg.c. For example we can imagine a function "move this temp whatever it is (mem, const, reg) in this register". If we declare it inline, we can leave the compiler do the few optimizations we have done. But indeed that's for 2.5.
On 07/25/2015 03:51 PM, Aurelien Jarno wrote: > On 2015-07-25 15:06, Richard Henderson wrote: >> On 07/24/2015 04:34 PM, Aurelien Jarno wrote: >>> ots->val_type = TEMP_VAL_CONST; >>> ots->val = ts->val; >>> + if (IS_DEAD_ARG(1)) { >>> + temp_dead(s, args[1]); >>> + } >> >> Aren't we also missing >> >> if (NEED_SYNC_ARG(0)) { >> temp_sync(s, args[0], allocated_regs); >> } >> >> along this path? > > I don't think so, I guess it's covered by the first part of this > function: > > | if (((NEED_SYNC_ARG(0) || ots->fixed_reg) && ts->val_type != TEMP_VAL_REG) > | || ts->val_type == TEMP_VAL_MEM) { > > It means after this block, a value that need to be synced will always > be in a register, including in the constant case. Quite right. Therefore, Reviewed-by: Richard Henderson <rth@twiddle.net> Do you want to go ahead and push this for 2.4? r~
On 2015-07-25 16:12, Richard Henderson wrote: > On 07/25/2015 03:51 PM, Aurelien Jarno wrote: > >On 2015-07-25 15:06, Richard Henderson wrote: > >>On 07/24/2015 04:34 PM, Aurelien Jarno wrote: > >>> ots->val_type = TEMP_VAL_CONST; > >>> ots->val = ts->val; > >>>+ if (IS_DEAD_ARG(1)) { > >>>+ temp_dead(s, args[1]); > >>>+ } > >> > >>Aren't we also missing > >> > >> if (NEED_SYNC_ARG(0)) { > >> temp_sync(s, args[0], allocated_regs); > >> } > >> > >>along this path? > > > >I don't think so, I guess it's covered by the first part of this > >function: > > > >| if (((NEED_SYNC_ARG(0) || ots->fixed_reg) && ts->val_type != TEMP_VAL_REG) > >| || ts->val_type == TEMP_VAL_MEM) { > > > >It means after this block, a value that need to be synced will always > >be in a register, including in the constant case. > > Quite right. Therefore, > > Reviewed-by: Richard Henderson <rth@twiddle.net> > > Do you want to go ahead and push this for 2.4? Yes, I think we should push it for 2.4. Do you want to do the pull request or should I do it?
diff --git a/tcg/tcg.c b/tcg/tcg.c index 7e088b1..9a2508b 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -1920,6 +1920,9 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def, } ots->val_type = TEMP_VAL_CONST; ots->val = ts->val; + if (IS_DEAD_ARG(1)) { + temp_dead(s, args[1]); + } } else { /* The code in the first if block should have moved the temp to a register. */
When tcg_reg_alloc_mov propagate a constant, we failed to correctly mark a temp as dead if the liveness analysis hints so. This fixes the following assert when configure with --enable-debug-tcg: qemu-x86_64: tcg/tcg.c:1827: tcg_reg_alloc_bb_end: Assertion `ts->val_type == TEMP_VAL_DEAD' failed. Cc: Richard Henderson <rth@twiddle.net> Reported-by: Richard Henderson <rth@twiddle.net> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- tcg/tcg.c | 3 +++ 1 file changed, 3 insertions(+) This is triggered by the patch "tcg/optimize: allow constant to have copies", but I guess it might be triggered other ways. Therefore it's a good candidate for 2.4.