| Message ID | 561F3D5B.4060709@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 15, 2015 at 7:44 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: > > > On 15/09/15 22:57, Richard Biener wrote: >> On Tue, Sep 8, 2015 at 2:00 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: >>> >>> Thanks for the review. >>> >>> On 07/09/15 23:20, Michael Matz wrote: >>>> Hi, >>>> >>>> On Mon, 7 Sep 2015, Kugan wrote: >>>> >>>>> Allow GIMPLE_DEBUG with values in promoted register. >>>> >>>> Patch does much more. >>>> >>> >>> Oops sorry. Copy and paste mistake. >>> >>> gcc/ChangeLog: >>> >>> 2015-09-07 Kugan Vivekanandarajah <kuganv@linaro.org> >>> >>> * cfgexpand.c (expand_debug_locations): Remove assert as now we are >>> also allowing values in promoted register. >>> * gimple-ssa-type-promote.c (fixup_uses): Allow GIMPLE_DEBUG to bind >>> values in promoted register. >>> * rtl.h (wi::int_traits ::decompose): Accept zero extended value >>> also. >>> >>> >>>>> gcc/ChangeLog: >>>>> >>>>> 2015-09-07 Kugan Vivekanandarajah <kuganv@linaro.org> >>>>> >>>>> * expr.c (expand_expr_real_1): Set proper SUNREG_PROMOTED_MODE for >>>>> SSA_NAME that was set by GIMPLE_CALL and assigned to another >>>>> SSA_NAME of same type. >>>> >>>> ChangeLog doesn't match patch, and patch contains dubious changes: >>>> >>>>> --- a/gcc/cfgexpand.c >>>>> +++ b/gcc/cfgexpand.c >>>>> @@ -5240,7 +5240,6 @@ expand_debug_locations (void) >>>>> tree value = (tree)INSN_VAR_LOCATION_LOC (insn); >>>>> rtx val; >>>>> rtx_insn *prev_insn, *insn2; >>>>> - machine_mode mode; >>>>> >>>>> if (value == NULL_TREE) >>>>> val = NULL_RTX; >>>>> @@ -5275,16 +5274,6 @@ expand_debug_locations (void) >>>>> >>>>> if (!val) >>>>> val = gen_rtx_UNKNOWN_VAR_LOC (); >>>>> - else >>>>> - { >>>>> - mode = GET_MODE (INSN_VAR_LOCATION (insn)); >>>>> - >>>>> - gcc_assert (mode == GET_MODE (val) >>>>> - || (GET_MODE (val) == VOIDmode >>>>> - && (CONST_SCALAR_INT_P (val) >>>>> - || GET_CODE (val) == CONST_FIXED >>>>> - || GET_CODE (val) == LABEL_REF))); >>>>> - } >>>>> >>>>> INSN_VAR_LOCATION_LOC (insn) = val; >>>>> prev_insn = PREV_INSN (insn); >>>> >>>> So it seems that the modes of the values location and the value itself >>>> don't have to match anymore, which seems dubious when considering how a >>>> debugger should load the value in question from the given location. So, >>>> how is it supposed to work? >>> >>> For example (simplified test-case from creduce): >>> >>> fn1() { >>> char a = fn1; >>> return a; >>> } >>> >>> --- test.c.142t.veclower21 2015-09-07 23:47:26.362201640 +0000 >>> +++ test.c.143t.promotion 2015-09-07 23:47:26.362201640 +0000 >>> @@ -5,13 +5,18 @@ >>> { >>> char a; >>> long int fn1.0_1; >>> + unsigned int _2; >>> int _3; >>> + unsigned int _5; >>> + char _6; >>> >>> <bb 2>: >>> fn1.0_1 = (long int) fn1; >>> - a_2 = (char) fn1.0_1; >>> - # DEBUG a => a_2 >>> - _3 = (int) a_2; >>> + _5 = (unsigned int) fn1.0_1; >>> + _2 = _5 & 255; >>> + # DEBUG a => _2 >>> + _6 = (char) _2; >>> + _3 = (int) _6; >>> return _3; >>> >>> } >>> >>> Please see that DEBUG now points to _2 which is a promoted mode. I am >>> assuming that the debugger would load required precision from promoted >>> register. May be I am missing the details but how else we can handle >>> this? Any suggestions? >> >> I would have expected the DEBUG insn to be adjusted as >> >> # DEBUG a => (char)_2 > > Thanks for the review. Please find the attached patch that attempts to > do this. I have also tested a version of this patch with gdb testsuite. > > As Michael wanted, I have also removed the changes in rtl.h and > promoting constants in GIMPLE_DEBUG. > > >> Btw, why do we have >> >>> + _6 = (char) _2; >>> + _3 = (int) _6; >> >> ? I'd have expected >> >> unsigned int _6 = SEXT <_2, 8> >> _3 = (int) _6; >> return _3; > > I am looking into it. > >> >> see my other mail about promotion of PARM_DECLs and RESULT_DECLs -- we should >> promote those as well. >> > > Just to be sure, are you referring to > https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00244.html > where you wanted an IPA pass to perform this. This is one of my dodo > after this. Please let me know if you wanted here is a different issue. No, that's the same issue. You remove @@ -5269,16 +5268,6 @@ expand_debug_locations (void) if (!val) val = gen_rtx_UNKNOWN_VAR_LOC (); - else - { - mode = GET_MODE (INSN_VAR_LOCATION (insn)); - - gcc_assert (mode == GET_MODE (val) - || (GET_MODE (val) == VOIDmode - && (CONST_SCALAR_INT_P (val) - || GET_CODE (val) == CONST_FIXED - || GET_CODE (val) == LABEL_REF))); - } which is in place to ensure the debug insns are "valid" in some form(?) On what kind of insn does the assert trigger with your patch so that you have to remove it? + + switch (TREE_CODE_CLASS (TREE_CODE (op))) + { + case tcc_exceptional: + case tcc_unary: + { Hmm. So when we promote _1 in _1 = ...; # DEBUG i = _1 + 7; to sth else it would probably best to instead of doing conversion of operands where necessary introduce a debug temporary like # DEBUG D#1 = (type-of-_1) replacement-of-_1; and replace debug uses of _1 with D#1 Richard. > > Thanks, > Kuganb
From 7cbcca8ebd03f60e16a55da4af3fc573f98d4086 Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Tue, 1 Sep 2015 08:40:40 +1000 Subject: [PATCH 5/7] debug stmt in widen mode --- gcc/cfgexpand.c | 11 ------- gcc/gimple-ssa-type-promote.c | 77 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 14 deletions(-) diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 357710b..be43f46 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -5234,7 +5234,6 @@ expand_debug_locations (void) tree value = (tree)INSN_VAR_LOCATION_LOC (insn); rtx val; rtx_insn *prev_insn, *insn2; - machine_mode mode; if (value == NULL_TREE) val = NULL_RTX; @@ -5269,16 +5268,6 @@ expand_debug_locations (void) if (!val) val = gen_rtx_UNKNOWN_VAR_LOC (); - else - { - mode = GET_MODE (INSN_VAR_LOCATION (insn)); - - gcc_assert (mode == GET_MODE (val) - || (GET_MODE (val) == VOIDmode - && (CONST_SCALAR_INT_P (val) - || GET_CODE (val) == CONST_FIXED - || GET_CODE (val) == LABEL_REF))); - } INSN_VAR_LOCATION_LOC (insn) = val; prev_insn = PREV_INSN (insn); diff --git a/gcc/gimple-ssa-type-promote.c b/gcc/gimple-ssa-type-promote.c index 513d20d..4034203 100644 --- a/gcc/gimple-ssa-type-promote.c +++ b/gcc/gimple-ssa-type-promote.c @@ -585,10 +585,81 @@ fixup_uses (tree use, tree promoted_type, tree old_type) { case GIMPLE_DEBUG: { - gsi = gsi_for_stmt (stmt); - gsi_remove (&gsi, true); - break; + tree op, new_op = NULL_TREE; + gdebug *copy = NULL, *gs = as_a <gdebug *> (stmt); + enum tree_code code; + + switch (gs->subcode) + { + case GIMPLE_DEBUG_BIND: + op = gimple_debug_bind_get_value (gs); + break; + case GIMPLE_DEBUG_SOURCE_BIND: + op = gimple_debug_source_bind_get_value (gs); + break; + default: + gcc_unreachable (); + } + + switch (TREE_CODE_CLASS (TREE_CODE (op))) + { + case tcc_exceptional: + case tcc_unary: + { + /* Convert DEBUG stmt of the form: + # DEBUG a => _2 + to + # DEBUG a => (char)_2 */ + new_op = build1 (CONVERT_EXPR, old_type, use); + break; + } + case tcc_binary: + code = TREE_CODE (op); + /* Convert the INTEGER_CST in tcc_binary to promoted_type, + if the expression is of kind that will be promoted. */ + if (code == LROTATE_EXPR + || code == RROTATE_EXPR + || code == COMPLEX_EXPR) + break; + else + { + tree op0 = TREE_OPERAND (op, 0); + tree op1 = TREE_OPERAND (op, 1); + if (TREE_CODE (op0) == INTEGER_CST) + op0 = convert_int_cst (promoted_type, op0, SIGNED); + if (TREE_CODE (op1) == INTEGER_CST) + op1 = convert_int_cst (promoted_type, op1, SIGNED); + new_op = build2 (TREE_CODE (op), promoted_type, op0, op1); + break; + } + default: + break; + } + + if (new_op) + { + if (gimple_debug_bind_p (stmt)) + { + copy = gimple_build_debug_bind + (gimple_debug_bind_get_var (stmt), + new_op, + stmt); + } + if (gimple_debug_source_bind_p (stmt)) + { + copy = gimple_build_debug_source_bind + (gimple_debug_source_bind_get_var (stmt), new_op, + stmt); + } + + if (copy) + { + gsi = gsi_for_stmt (stmt); + gsi_replace (&gsi, copy, false); + } + } } + break; case GIMPLE_ASM: case GIMPLE_CALL: -- 1.9.1