[PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[LIU Zhiwei]

TS_DEAD means we will release the register allocated for this temporary. But
at basic block ending, we can still use the allocted register.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index bb52bc060b..0c93e6e6f8 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, int nt)
         case TEMP_FIXED:
         case TEMP_GLOBAL:
         case TEMP_TB:
-            state = TS_DEAD | TS_MEM;
+            state = TS_MEM;
             break;
         case TEMP_EBB:
         case TEMP_CONST:
-- 
2.17.1

Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[Richard Henderson]

On 3/20/23 21:53, LIU Zhiwei wrote:
> TS_DEAD means we will release the register allocated for this temporary. But
> at basic block ending, we can still use the allocted register.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Test case?


r~

> ---
>   tcg/tcg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index bb52bc060b..0c93e6e6f8 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, int nt)
>           case TEMP_FIXED:
>           case TEMP_GLOBAL:
>           case TEMP_TB:
> -            state = TS_DEAD | TS_MEM;
> +            state = TS_MEM;
>               break;
>           case TEMP_EBB:
>           case TEMP_CONST:

Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[LIU Zhiwei]

On 2023/3/21 14:06, Richard Henderson wrote:
> On 3/20/23 21:53, LIU Zhiwei wrote:
>> TS_DEAD means we will release the register allocated for this 
>> temporary. But
>> at basic block ending, we can still use the allocted register.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>
> Test case?

I have run an Ubuntu image after this patch. It can boot.

But I can't find a direct test case.  Because the IRs supported with 
flags TCG_OPF_BB_END do not have  input or output parameter, such as the 
set_label or br.

Best Regards,
Zhiwei

>
>
> r~
>
>> ---
>>   tcg/tcg.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>> index bb52bc060b..0c93e6e6f8 100644
>> --- a/tcg/tcg.c
>> +++ b/tcg/tcg.c
>> @@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, 
>> int nt)
>>           case TEMP_FIXED:
>>           case TEMP_GLOBAL:
>>           case TEMP_TB:
>> -            state = TS_DEAD | TS_MEM;
>> +            state = TS_MEM;
>>               break;
>>           case TEMP_EBB:
>>           case TEMP_CONST:

Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[Richard Henderson]

On 3/20/23 23:44, LIU Zhiwei wrote:
> 
> On 2023/3/21 14:06, Richard Henderson wrote:
>> On 3/20/23 21:53, LIU Zhiwei wrote:
>>> TS_DEAD means we will release the register allocated for this temporary. But
>>> at basic block ending, we can still use the allocted register.
>>>
>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>>
>> Test case?
> 
> I have run an Ubuntu image after this patch. It can boot.

That's surprising.  I would have expected an assert with --enable-debug-tcg, but this 
appears to be an oversight in tcg_reg_alloc_bb_end.  We only validate the liveness data 
for TEMP_EBB and TEMP_CONST, but not TEMP_TB or TEMP_GLOBAL.

> But I can't find a direct test case.  Because the IRs supported with flags TCG_OPF_BB_END 
> do not have  input or output parameter, such as the set_label or br.

That's exactly why we want all GLOBAL and TB to be DEAD | MEM, so that they're saved back 
to their home slots and released from their registers.

The register allocator for TCG does not work across extended basic blocks.  Importantly, 
if you have a forward branch like so:


	g1 = func(a)
	brcond ..., L1

	stuff
	g2 = func(b)	
	g1 = g2
	discard g2

L1:

What value should g1->reg have at L1?  The allocator does not do the global control flow 
and allocation required to ensure that g1->reg is the same at the brcond and at the label.

Nominally, I would have expected one value for g1->reg at the branch, a different value 
for g2->reg in the second BB, and for the assignment to steal g2->reg and move it to 
g1->reg (per tcg_reg_alloc_mov of an IS_DEAD_ARG temp).  Which would result in an 
incorrect allocation at L1.

What are you attempting to do?  Is this just guesswork?


r~