Patchwork Try to canonicalize MEM_REFs that are put into MEM_EXPRs for DEBUG_INSNs (PR debug/47283)

login
register
mail settings
Submitter Jakub Jelinek
Date March 2, 2011, 5:40 p.m.
Message ID <20110302174015.GR30899@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/85115/
State New
Headers show

Comments

Jakub Jelinek - March 2, 2011, 5:40 p.m.
Hi!

The pr47283.C testcase was still failing on ia64-linux, there
the var in question was given a stack slot, but ICEs were caused by
non-canonical MEM_REF in MEM_EXPR of DEBUG_INSN MEM.  In particular
there was MEM_REF <ADDR_EXPR <COMPONENT_REF <...> >, 0> on which aliasing
code ICEd.  This patch tries to canonicalize it, and if the canonicalization
wasn't sufficient, uses just type and not the MEM_REF for
set_mem_attributes.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2011-03-02  Jakub Jelinek  <jakub@redhat.com>

	PR debug/47283
	* cfgexpand.c (expand_debug_expr) <case MEM_REF>: If MEM_REF
	first operand is not is_gimple_mem_ref_addr, try to fold it.
	If the operand still isn't is_gimple_mem_ref_addr, pass just
	its type to set_mem_attributes.


	Jakub
Richard Henderson - March 3, 2011, 5:31 a.m.
On 03/03/2011 03:40 AM, Jakub Jelinek wrote:
> Hi!
> 
> The pr47283.C testcase was still failing on ia64-linux, there
> the var in question was given a stack slot, but ICEs were caused by
> non-canonical MEM_REF in MEM_EXPR of DEBUG_INSN MEM.  In particular
> there was MEM_REF <ADDR_EXPR <COMPONENT_REF <...> >, 0> on which aliasing
> code ICEd.  This patch tries to canonicalize it, and if the canonicalization
> wasn't sufficient, uses just type and not the MEM_REF for
> set_mem_attributes.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2011-03-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/47283
> 	* cfgexpand.c (expand_debug_expr) <case MEM_REF>: If MEM_REF
> 	first operand is not is_gimple_mem_ref_addr, try to fold it.
> 	If the operand still isn't is_gimple_mem_ref_addr, pass just
> 	its type to set_mem_attributes.
> 
> --- gcc/cfgexpand.c.jj	2011-02-21 15:37:42.000000000 +0100
> +++ gcc/cfgexpand.c	2011-03-02 11:05:32.000000000 +0100
> @@ -2578,11 +2578,25 @@ expand_debug_expr (tree exp)
>        }
>  
>      case MEM_REF:
> +      if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0)))
> +	{
> +	  tree newexp = fold_binary (MEM_REF, TREE_TYPE (exp),
> +				     TREE_OPERAND (exp, 0),
> +				     TREE_OPERAND (exp, 1));
> +	  if (newexp)
> +	    exp = newexp;
> +	}
> +      /* FALLTHROUGH */
>      case INDIRECT_REF:

Do you really want to fallthru with newexp here?
In theory it could have folded to anything.  My guess is that
you want to restart instead...


r~
Jakub Jelinek - March 3, 2011, 7:22 a.m.
On Thu, Mar 03, 2011 at 03:31:55PM +1000, Richard Henderson wrote:
> On 03/03/2011 03:40 AM, Jakub Jelinek wrote:
> >      case MEM_REF:
> > +      if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0)))
> > +	{
> > +	  tree newexp = fold_binary (MEM_REF, TREE_TYPE (exp),
> > +				     TREE_OPERAND (exp, 0),
> > +				     TREE_OPERAND (exp, 1));
> > +	  if (newexp)
> > +	    exp = newexp;
> > +	}
> > +      /* FALLTHROUGH */
> >      case INDIRECT_REF:
> 
> Do you really want to fallthru with newexp here?
> In theory it could have folded to anything.  My guess is that
> you want to restart instead...

fold_binary only optimizes:
/* MEM[&MEM[p, CST1], CST2] -> MEM[p, CST1 + CST2].  */
and
/* MEM[&a.b, CST2] -> MEM[&a, offsetof (a, b) + CST2].  */

But if you prefer, I can certainly do there
	if (newexp)
	  return expand_debug_expr (newexp);
instead.

	Jakub
Richard Guenther - March 3, 2011, 10:02 a.m.
On Thu, Mar 3, 2011 at 8:22 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 03, 2011 at 03:31:55PM +1000, Richard Henderson wrote:
>> On 03/03/2011 03:40 AM, Jakub Jelinek wrote:
>> >      case MEM_REF:
>> > +      if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0)))
>> > +   {
>> > +     tree newexp = fold_binary (MEM_REF, TREE_TYPE (exp),
>> > +                                TREE_OPERAND (exp, 0),
>> > +                                TREE_OPERAND (exp, 1));
>> > +     if (newexp)
>> > +       exp = newexp;
>> > +   }
>> > +      /* FALLTHROUGH */
>> >      case INDIRECT_REF:
>>
>> Do you really want to fallthru with newexp here?
>> In theory it could have folded to anything.  My guess is that
>> you want to restart instead...
>
> fold_binary only optimizes:
> /* MEM[&MEM[p, CST1], CST2] -> MEM[p, CST1 + CST2].  */
> and
> /* MEM[&a.b, CST2] -> MEM[&a, offsetof (a, b) + CST2].  */
>
> But if you prefer, I can certainly do there
>        if (newexp)
>          return expand_debug_expr (newexp);
> instead.

It's probably more safe, at least for 4.7, in case we ever start to do
constant folding here.

Btw, I don't really like another caller of set_mem_attributes that
passes a type rather than an expression ... OTOH, the folding
should never fail to generate a canonical MEM ref (I bet asserting
that the MEM_REF is a proper one after folding passes bootstrap
just ok), so maybe just drop the debug expr on the floor in this
case (eventually ICE when checking is enabled)?  At least I can't
see how we could end up with a non-canonicalizable address.

Richard.

Patch

--- gcc/cfgexpand.c.jj	2011-02-21 15:37:42.000000000 +0100
+++ gcc/cfgexpand.c	2011-03-02 11:05:32.000000000 +0100
@@ -2578,11 +2578,25 @@  expand_debug_expr (tree exp)
       }
 
     case MEM_REF:
+      if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0)))
+	{
+	  tree newexp = fold_binary (MEM_REF, TREE_TYPE (exp),
+				     TREE_OPERAND (exp, 0),
+				     TREE_OPERAND (exp, 1));
+	  if (newexp)
+	    exp = newexp;
+	}
+      /* FALLTHROUGH */
     case INDIRECT_REF:
       op0 = expand_debug_expr (TREE_OPERAND (exp, 0));
       if (!op0)
 	return NULL;
 
+      if (POINTER_TYPE_P (TREE_TYPE (exp)))
+	as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
+      else
+	as = ADDR_SPACE_GENERIC;
+
       if (TREE_CODE (exp) == MEM_REF)
 	{
 	  if (GET_CODE (op0) == DEBUG_IMPLICIT_PTR
@@ -2597,12 +2611,10 @@  expand_debug_expr (tree exp)
 	    return NULL;
 
 	  op0 = plus_constant (op0, INTVAL (op1));
-	}
 
-      if (POINTER_TYPE_P (TREE_TYPE (exp)))
-	as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
-      else
-	as = ADDR_SPACE_GENERIC;
+	  if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0)))
+	    exp = TREE_TYPE (exp);
+	}
 
       op0 = convert_debug_memory_address (targetm.addr_space.address_mode (as),
 					  op0, as);