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 <kug...@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 <kug...@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?
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
