Re: Fix PR43077, debug regression with SSA expand

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Feb 23, 2010 at 3:59 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Fri, 19 Feb 2010, Michael Matz wrote:
>
>> Due to some discussion I retract this one for now. ?It would do the wrong
>> thing for such situation:
>>
>> a_1 = b_2 + c_3 ? ?// assume a_1 is TERed
>> ... use(a_1) ? ? ? // only real use of a_1
>> b_3 = ...
>> #DEBUG something => f(a_1)
>>
>> When forwarding the RHS "b_2 + c_3" into the debug use we might have
>> clobbered b_2 already, namely when b_3 and b_2 can be coalesced. ?I'm now
>> planning to use debug temps for this situation.
>
> Like so. ?Regstrapped on x86_64-linux, no regressions (and since then only
> added the comments). ?Okay for trunk?

Ok.

Thanks,
Richard.

>
> Ciao,
> Michael.
> --
> ? ? ? ?PR debug/43077
> ? ? ? ?* cfgexpand (expand_debug_expr): Expand TERed ssa names in place.
> ? ? ? ?(expand_gimple_basic_block): Generate and use debug temps if there
> ? ? ? ?are debug uses left after the last real use of TERed ssa names.
> ? ? ? ?Unlink debug immediate uses when they are expanded.
>
> testsuite/
> 2010-02-19 ?Jakub Jelinek ?<jakub@redhat.com>
> ? ? ? ?PR debug/43077
> ? ? ? ?* gcc.dg/guality/pr43077-1.c: New test.
>
> Index: cfgexpand.c
> ===================================================================
> *** cfgexpand.c (revision 156895)
> --- cfgexpand.c (working copy)
> *************** expand_debug_expr (tree exp)
> *** 2338,2344 ****
>
> ? ? ? ?if (inner_mode == VOIDmode)
> ? ? ? ? ?{
> ! ? ? ? ? ? inner_mode = TYPE_MODE (TREE_TYPE (TREE_OPERAND (exp, 0)));
> ? ? ? ? ? ?if (mode == inner_mode)
> ? ? ? ? ? ? ?return op0;
> ? ? ? ? ?}
> --- 2338,2347 ----
>
> ? ? ? ?if (inner_mode == VOIDmode)
> ? ? ? ? ?{
> ! ? ? ? ? ? if (TREE_CODE (exp) == SSA_NAME)
> ! ? ? ? ? ? ? inner_mode = TYPE_MODE (TREE_TYPE (exp));
> ! ? ? ? ? ? else
> ! ? ? ? ? ? ? inner_mode = TYPE_MODE (TREE_TYPE (TREE_OPERAND (exp, 0)));
> ? ? ? ? ? ?if (mode == inner_mode)
> ? ? ? ? ? ? ?return op0;
> ? ? ? ? ?}
> *************** expand_debug_expr (tree exp)
> *** 2354,2359 ****
> --- 2357,2363 ----
> ? ? ? ? ?}
> ? ? ? ?else if (FLOAT_MODE_P (mode))
> ? ? ? ? ?{
> + ? ? ? ? ? gcc_assert (TREE_CODE (exp) != SSA_NAME);
> ? ? ? ? ? ?if (TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0))))
> ? ? ? ? ? ? ?op0 = simplify_gen_unary (UNSIGNED_FLOAT, mode, op0, inner_mode);
> ? ? ? ? ? ?else
> *************** expand_debug_expr (tree exp)
> *** 2886,2899 ****
>
> ? ? ?case SSA_NAME:
> ? ? ? ?{
> ! ? ? ? int part = var_to_partition (SA.map, exp);
>
> ! ? ? ? if (part == NO_PARTITION)
> ! ? ? ? ? return NULL;
>
> ! ? ? ? gcc_assert (part >= 0 && (unsigned)part < SA.map->num_partitions);
>
> ! ? ? ? op0 = SA.partition_to_pseudo[part];
> ? ? ? ?goto adjust_mode;
> ? ? ? ?}
>
> --- 2890,2913 ----
>
> ? ? ?case SSA_NAME:
> ? ? ? ?{
> ! ? ? ? gimple g = get_gimple_for_ssa_name (exp);
> ! ? ? ? if (g)
> ! ? ? ? ? {
> ! ? ? ? ? ? op0 = expand_debug_expr (gimple_assign_rhs_to_tree (g));
> ! ? ? ? ? ? if (!op0)
> ! ? ? ? ? ? ? return NULL;
> ! ? ? ? ? }
> ! ? ? ? else
> ! ? ? ? ? {
> ! ? ? ? ? ? int part = var_to_partition (SA.map, exp);
>
> ! ? ? ? ? ? if (part == NO_PARTITION)
> ! ? ? ? ? ? ? return NULL;
>
> ! ? ? ? ? ? gcc_assert (part >= 0 && (unsigned)part < SA.map->num_partitions);
>
> ! ? ? ? ? ? op0 = SA.partition_to_pseudo[part];
> ! ? ? ? ? }
> ? ? ? ?goto adjust_mode;
> ? ? ? ?}
>
> *************** expand_gimple_basic_block (basic_block b
> *** 3050,3055 ****
> --- 3064,3168 ----
> ? ? ? ?basic_block new_bb;
>
> ? ? ? ?stmt = gsi_stmt (gsi);
> +
> + ? ? ? /* If this statement is a non-debug one, and we generate debug
> + ? ? ? ? ?insns, then this one might be the last real use of a TERed
> + ? ? ? ?SSA_NAME, but where there are still some debug uses further
> + ? ? ? ?down. ?Expanding the current SSA name in such further debug
> + ? ? ? ?uses by their RHS might lead to wrong debug info, as coalescing
> + ? ? ? ?might make the operands of such RHS be placed into the same
> + ? ? ? ?pseudo as something else. ?Like so:
> + ? ? ? ? ?a_1 = a_0 + 1; ? // Assume a_1 is TERed and a_0 is dead
> + ? ? ? ? ?use(a_1);
> + ? ? ? ? ?a_2 = ...
> + ? ? ? ? ? ?#DEBUG ... => a_1
> + ? ? ? ?As a_0 and a_2 don't overlap in lifetime, assume they are coalesced.
> + ? ? ? ?If we now would expand a_1 by it's RHS (a_0 + 1) in the debug use,
> + ? ? ? ?the write to a_2 would actually have clobbered the place which
> + ? ? ? ?formerly held a_0.
> +
> + ? ? ? ?So, instead of that, we recognized the situation, and generate
> + ? ? ? ?debug temporaries at the last real use of TERed SSA names:
> + ? ? ? ? ?a_1 = a_0 + 1; ? // Assume a_1 is TERed and a_0 is dead
> + ? ? ? ? ? ?#DEBUG #D1 => a_1
> + ? ? ? ? ?use(a_1);
> + ? ? ? ? ?a_2 = ...
> + ? ? ? ? ? ?#DEBUG ... => #D1
> + ? ? ? ?*/
> + ? ? ? if (MAY_HAVE_DEBUG_INSNS
> + ? ? ? ? && SA.values
> + ? ? ? ? && !is_gimple_debug (stmt))
> + ? ? ? {
> + ? ? ? ? ssa_op_iter iter;
> + ? ? ? ? tree op;
> + ? ? ? ? gimple def;
> +
> + ? ? ? ? location_t sloc = get_curr_insn_source_location ();
> + ? ? ? ? tree sblock = get_curr_insn_block ();
> +
> + ? ? ? ? /* Look for SSA names that have their last use here (TERed
> + ? ? ? ? ? ?names always have only one real use). ?*/
> + ? ? ? ? FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> + ? ? ? ? ? if ((def = get_gimple_for_ssa_name (op)))
> + ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? imm_use_iterator imm_iter;
> + ? ? ? ? ? ? ? use_operand_p use_p;
> + ? ? ? ? ? ? ? bool have_debug_uses = false;
> +
> + ? ? ? ? ? ? ? FOR_EACH_IMM_USE_FAST (use_p, imm_iter, op)
> + ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? if (gimple_debug_bind_p (USE_STMT (use_p)))
> + ? ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? ? have_debug_uses = true;
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? if (have_debug_uses)
> + ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? /* OP is a TERed SSA name, with DEF it's defining
> + ? ? ? ? ? ? ? ? ? ? ?statement, and where OP is used in further debug
> + ? ? ? ? ? ? ? ? ? ? ?instructions. ?Generate a debug temporary, and
> + ? ? ? ? ? ? ? ? ? ? ?replace all uses of OP in debug insns with that
> + ? ? ? ? ? ? ? ? ? ? ?temporary. ?*/
> + ? ? ? ? ? ? ? ? ? gimple debugstmt;
> + ? ? ? ? ? ? ? ? ? tree value = gimple_assign_rhs_to_tree (def);
> + ? ? ? ? ? ? ? ? ? tree vexpr = make_node (DEBUG_EXPR_DECL);
> + ? ? ? ? ? ? ? ? ? rtx val;
> + ? ? ? ? ? ? ? ? ? enum machine_mode mode;
> +
> + ? ? ? ? ? ? ? ? ? set_curr_insn_source_location (gimple_location (def));
> + ? ? ? ? ? ? ? ? ? set_curr_insn_block (gimple_block (def));
> +
> + ? ? ? ? ? ? ? ? ? DECL_ARTIFICIAL (vexpr) = 1;
> + ? ? ? ? ? ? ? ? ? TREE_TYPE (vexpr) = TREE_TYPE (value);
> + ? ? ? ? ? ? ? ? ? if (DECL_P (value))
> + ? ? ? ? ? ? ? ? ? ? mode = DECL_MODE (value);
> + ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? mode = TYPE_MODE (TREE_TYPE (value));
> + ? ? ? ? ? ? ? ? ? DECL_MODE (vexpr) = mode;
> +
> + ? ? ? ? ? ? ? ? ? val = gen_rtx_VAR_LOCATION
> + ? ? ? ? ? ? ? ? ? ? ? (mode, vexpr, (rtx)value, VAR_INIT_STATUS_INITIALIZED);
> +
> + ? ? ? ? ? ? ? ? ? val = emit_debug_insn (val);
> +
> + ? ? ? ? ? ? ? ? ? FOR_EACH_IMM_USE_STMT (debugstmt, imm_iter, op)
> + ? ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? ? if (!gimple_debug_bind_p (debugstmt))
> + ? ? ? ? ? ? ? ? ? ? ? ? continue;
> +
> + ? ? ? ? ? ? ? ? ? ? ? FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
> + ? ? ? ? ? ? ? ? ? ? ? ? SET_USE (use_p, vexpr);
> +
> + ? ? ? ? ? ? ? ? ? ? ? update_stmt (debugstmt);
> + ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? }
> + ? ? ? ? set_curr_insn_source_location (sloc);
> + ? ? ? ? set_curr_insn_block (sblock);
> + ? ? ? }
> +
> ? ? ? ?currently_expanding_gimple_stmt = stmt;
>
> ? ? ? ?/* Expand this statement, then evaluate the resulting RTL and
> *************** expand_gimple_basic_block (basic_block b
> *** 3102,3107 ****
> --- 3215,3227 ----
> ? ? ? ? ? ? ? ? ?INSN_VAR_LOCATION_LOC (val) = (rtx)value;
> ? ? ? ? ? ? ? ?}
>
> + ? ? ? ? ? ? /* In order not to generate too many debug temporaries,
> + ? ? ? ? ? ? ? ?we delink all uses of debug statements we already expanded.
> + ? ? ? ? ? ? ? ?Therefore debug statements between definition and real
> + ? ? ? ? ? ? ? ?use of TERed SSA names will continue to use the SSA name,
> + ? ? ? ? ? ? ? ?and not be replaced with debug temps. ?*/
> + ? ? ? ? ? ? delink_stmt_imm_use (stmt);
> +
> ? ? ? ? ? ? ?gsi = nsi;
> ? ? ? ? ? ? ?gsi_next (&nsi);
> ? ? ? ? ? ? ?if (gsi_end_p (nsi))
> Index: testsuite/gcc.dg/guality/pr43077-1.c
> ===================================================================
> *** testsuite/gcc.dg/guality/pr43077-1.c ? ? ? ?(revision 0)
> --- testsuite/gcc.dg/guality/pr43077-1.c ? ? ? ?(revision 0)
> ***************
> *** 0 ****
> --- 1,55 ----
> + /* PR debug/43077 */
> + /* { dg-do run } */
> + /* { dg-options "-g" } */
> +
> + int varb;
> +
> + int __attribute__((noinline))
> + fn1 (void)
> + {
> + ? int vara = (varb == 3); ? ? ? ? ? ? /* { dg-final { gdb-test 11 "vara" "0" } } */
> + ? asm volatile ("" : : "g" (vara)); ? /* { dg-final { gdb-test 11 "varb" "2" } } */
> + ? return 0;
> + }
> +
> + int __attribute__((noinline))
> + fn2 (void)
> + {
> + ? int vara = (varb == 3); ? ? ? ? ? ? /* { dg-final { gdb-test 19 "vara" "1" } } */
> + ? asm volatile ("" : : "g" (vara)); ? /* { dg-final { gdb-test 19 "varb" "3" } } */
> + ? return 0;
> + }
> +
> + int __attribute__((noinline))
> + foo (unsigned long *p, unsigned long *q)
> + {
> + ? int ret;
> + ? asm volatile ("" : "=r" (ret), "=r" (*p), "=r" (*q) : "0" (1), "1" (2), "2" (3));
> + ? return ret;
> + }
> +
> + int __attribute__((noinline))
> + fn3 (void)
> + {
> + ? unsigned long a = 0, b = 0, c = 0;
> + ? a = foo (&b, &c);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* { dg-final { gdb-test 42 "a" "1" } } */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* { dg-final { gdb-test 42 "b" "2" } } */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* { dg-final { gdb-test 42 "c" "3" } } */
> + ? unsigned long vara = a; ? ? ? ? ? ? /* { dg-final { gdb-test 42 "vara" "1" } } */
> + ? unsigned long varb = b; ? ? ? ? ? ? /* { dg-final { gdb-test 42 "varb" "2" } } */
> + ? unsigned long varc = c; ? ? ? ? ? ? /* { dg-final { gdb-test 42 "varc" "3" } } */
> + ? asm volatile ("" : : "g" (vara), "g" (varb), "g" (varc));
> + ? return a;
> + }
> +
> + int
> + main (void)
> + {
> + ? asm volatile ("" : "=r" (varb) : "0" (2));
> + ? fn1 ();
> + ? asm volatile ("" : "=r" (varb) : "0" (3));
> + ? fn2 ();
> + ? fn3 ();
> + ? return 0;
> + }
>