[PATCH] tcg: Revert "tcg/optimize: Flush data at labels not TCG_OPF_BB_END"

[PATCH] tcg: Revert "tcg/optimize: Flush data at labels not TCG_OPF_BB_END"

[Richard Henderson]

This reverts commit cd0372c515c4732d8bd3777cdd995c139c7ed7ea.

The patch is incorrect in that it retains copies between globals and
non-local temps, and non-local temps still die at the end of the BB.

Failing test case for hppa:

	.globl	_start
_start:
	cmpiclr,=	0x24,%r19,%r0
	cmpiclr,<>	0x2f,%r19,%r19

 ---- 00010057 0001005b
 movi_i32 tmp0,$0x24
 sub_i32 tmp1,tmp0,r19
 mov_i32 tmp2,tmp0
 mov_i32 tmp3,r19
 movi_i32 tmp1,$0x0

 ---- 0001005b 0001005f
 brcond_i32 tmp2,tmp3,eq,$L1
 movi_i32 tmp0,$0x2f
 sub_i32 tmp1,tmp0,r19
 mov_i32 tmp2,tmp0
 mov_i32 tmp3,r19
 movi_i32 tmp1,$0x0
 mov_i32 r19,tmp1
 setcond_i32 psw_n,tmp2,tmp3,ne
 set_label $L1

In this case, both copies of "mov_i32 tmp3,r19" are removed.  The
second because opt thought it was redundant.  The first is removed
later by liveness because tmp3 is known to be dead.  This leaves
the setcond_i32 with an uninitialized input.

Revert the entire patch for 5.2, and a proper optimization across
the branch may be considered for the next development cycle.

Reported-by: qemu@igor2.repo.hu
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/optimize.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 9952c28bdc..220f4601d5 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -1484,30 +1484,29 @@ void tcg_optimize(TCGContext *s)
                     }
                 }
             }
-            /* fall through */
+            goto do_reset_output;
 
         default:
         do_default:
-            /*
-             * Default case: we know nothing about operation (or were unable
-             * to compute the operation result) so no propagation is done.
-             */
-            for (i = 0; i < nb_oargs; i++) {
-                reset_temp(op->args[i]);
-                /*
-                 * Save the corresponding known-zero bits mask for the
-                 * first output argument (only one supported so far).
-                 */
-                if (i == 0) {
-                    arg_info(op->args[i])->mask = mask;
+            /* Default case: we know nothing about operation (or were unable
+               to compute the operation result) so no propagation is done.
+               We trash everything if the operation is the end of a basic
+               block, otherwise we only trash the output args.  "mask" is
+               the non-zero bits mask for the first output arg.  */
+            if (def->flags & TCG_OPF_BB_END) {
+                bitmap_zero(temps_used.l, nb_temps);
+            } else {
+        do_reset_output:
+                for (i = 0; i < nb_oargs; i++) {
+                    reset_temp(op->args[i]);
+                    /* Save the corresponding known-zero bits mask for the
+                       first output argument (only one supported so far). */
+                    if (i == 0) {
+                        arg_info(op->args[i])->mask = mask;
+                    }
                 }
             }
             break;
-
-        case INDEX_op_set_label:
-            /* Trash everything at the start of a new extended bb. */
-            bitmap_zero(temps_used.l, nb_temps);
-            break;
         }
 
         /* Eliminate duplicate and redundant fence instructions.  */
-- 
2.25.1

Re: [PATCH] tcg: Revert "tcg/optimize: Flush data at labels not TCG_OPF_BB_END"

[Peter Maydell]

On Tue, 3 Nov 2020 at 21:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This reverts commit cd0372c515c4732d8bd3777cdd995c139c7ed7ea.
>
> The patch is incorrect in that it retains copies between globals and
> non-local temps, and non-local temps still die at the end of the BB.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I assume you're going to put this in a pullreq at some point
before rc1?

thanks
-- PMM

Re: [PATCH] tcg: Revert "tcg/optimize: Flush data at labels not TCG_OPF_BB_END"

[Richard Henderson]

On 11/4/20 10:12 AM, Peter Maydell wrote:
> On Tue, 3 Nov 2020 at 21:38, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This reverts commit cd0372c515c4732d8bd3777cdd995c139c7ed7ea.
>>
>> The patch is incorrect in that it retains copies between globals and
>> non-local temps, and non-local temps still die at the end of the BB.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I assume you're going to put this in a pullreq at some point
> before rc1?

Yep.  I have one other bug fix as well.


r~