| Message ID | 55EE2523.6000209@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 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; see my other mail about promotion of PARM_DECLs and RESULT_DECLs -- we should promote those as well. Richard. > In this particular simplified case, we do have _6 but we might not in > all the case. > > >> >> And this change: >> >>> --- a/gcc/rtl.h >>> +++ b/gcc/rtl.h >>> @@ -2100,6 +2100,8 @@ wi::int_traits <rtx_mode_t>::decompose (HOST_WIDE_INT*, >>> targets is 1 rather than -1. */ >>> gcc_checking_assert (INTVAL (x.first) >>> == sext_hwi (INTVAL (x.first), precision) >>> + || INTVAL (x.first) >>> + == (INTVAL (x.first) & ((1 << precision) - 1)) >>> || (x.second == BImode && INTVAL (x.first) == 1)); >>> >>> return wi::storage_ref (&INTVAL (x.first), 1, precision); >> >> implies that wide_ints are not always sign-extended anymore after you >> changes. That's a fundamental assumption, so removing that assert implies >> that you somehow created non-canonical wide_ints, and those will cause >> bugs elsewhere in the code. Don't just remove asserts, they are usually >> there for a reason, and without accompanying changes those reasons don't >> go away. >> > > > This comes from GIMPLE_DEBUG. If this assumption should always hold, I > will fix it there. > > Thanks, > Kugan
--- 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; }