Re: [5/7] Allow gimple debug stmt in widen mode

[Kugan <kugan.vivekanandarajah@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 4197 bytes --]



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.


Thanks,
Kuganb

[-- Attachment #2: 0005-debug-stmt-in-widen-mode.patch --]
[-- Type: text/x-diff, Size: 3483 bytes --]

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