[Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Aurelien Jarno]

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.

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. */
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Aurelien Jarno]

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.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH for-2.4] tcg: correctly mark dead inputs for mov with a constant

[Aurelien Jarno]

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?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net