expand_expr tweaks to fix PR57134

expand_expr tweaks to fix PR57134

[Alan Modra]

The following patch fixes PR57134 by
a) excluding bitfield expansion when EXPAND_MEMORY, and
b) passing down the EXPAND_MEMORY modifier in a couple of places where
this does not currently happen on recursive calls to expand_expr().

(a) has precedent elsewhere in expr.c, eg. see expand_expr_real_1
<MEM_REF>, (b) is a little difficult to justify except to claim
there's no logical reason why it should be excluded nowadays.  Hand
waving argument follows:

Of the seven expand_expr() modifiers, recursive calls made to handle
inner references of aggregate types currently exclude EXPAND_SUM,
EXPAND_WRITE, and EXPAND_MEMORY from the modifier passed.  I suppose
EXPAND_SUM is excluded so that the inner expand_expr() won't return a
PLUS or MULT when we might be adding a further offset, but I can't see
why the other two modifiers are not passed.  Digging through the
history, it looks like EXPAND_WRITE may have been missed by accident,
and EXPAND_MEMORY used to be an entirely different animal.  kenner was
responsible for the original recursive call that passed down
EXPAND_NORMAL, EXPAND_CONST_ADDRESS and EXPAND_INITIALIZER when expr.h
looked like:

 14612     kenner    EXPAND_MEMORY_USE_* are explained below.  */
   406     kenner enum expand_modifier {EXPAND_NORMAL, EXPAND_SUM,
 14612     kenner                     EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER,
 14612     kenner                     EXPAND_MEMORY_USE_WO, EXPAND_MEMORY_USE_RW,
 14612     kenner                     EXPAND_MEMORY_USE_BAD, EXPAND_MEMORY_USE_DONT};
   406     kenner 
 14612     kenner /* Argument for chkr_* functions.
 14612     kenner    MEMORY_USE_RO: the pointer reads memory.
 14612     kenner    MEMORY_USE_WO: the pointer writes to memory.
 14612     kenner    MEMORY_USE_RW: the pointer modifies memory (ie it reads and writes). An
 14612     kenner                   example is (*ptr)++
 14612     kenner    MEMORY_USE_BAD: use this if you don't know the behavior of the pointer, or
 14612     kenner                    if you know there are no pointers.  Using an INDIRECT_REF
 14612     kenner                    with MEMORY_USE_BAD will abort.
 14612     kenner    MEMORY_USE_TW: just test for writing, without update.  Special.
 14612     kenner    MEMORY_USE_DONT: the memory is neither read nor written.  This is used by
 14612     kenner                  '->' and '.'.  */

So I think passing EXPAND_MEMORY and EXPAND_WRITE down ought to be
good.  The VIEW_CONVERT_EXPR case really doesn't need to exclude
EXPAND_SUM either, since it doesn't allow an offset, but I made it the
same as the inner ref case so that when/if someone attends to
	/* ??? We should work harder and deal with non-zero offsets.  */
it doesn't cause a surprise.  Really, a lot of this code needs
attention, for example, I think SSA made EXPAND_STACK_PARM and all of
the related code in calls.c dealing with libcalls when expanding args,
unnecessary.

Bootstrapped and regression tested powerpc64-linux.  OK to apply?

	PR middle-end/57134
	* expr.c (expand_expr_real_1 <normal_inner_ref>): Pass
	EXPAND_MEMORY and EXPAND_WRITE to recursive call.  Don't use
	bitfield expansion when EXPAND_MEMORY.
	(expand_expr_real_1 <VIEW_CONVERT_EXPR>): Pass modifier likewise.
	

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 199940)
+++ gcc/expr.c	(working copy)
@@ -9918,12 +9919,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 			  && modifier != EXPAND_STACK_PARM
 			  ? target : NULL_RTX),
 			 VOIDmode,
-			 (modifier == EXPAND_INITIALIZER
-			  || modifier == EXPAND_CONST_ADDRESS
-			  || modifier == EXPAND_STACK_PARM)
-			 ? modifier : EXPAND_NORMAL);
+			 modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
 
-
 	/* If the bitfield is volatile, we want to access it in the
 	   field's mode, not the computed mode.
 	   If a MEM has VOIDmode (external with incomplete type),
@@ -10081,6 +10078,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 		      || (MEM_P (op0)
 			  && (MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode1)
 			      || (bitpos % GET_MODE_ALIGNMENT (mode1) != 0))))
+		     && modifier != EXPAND_MEMORY
 		     && ((modifier == EXPAND_CONST_ADDRESS
 			  || modifier == EXPAND_INITIALIZER)
 			 ? STRICT_ALIGNMENT
@@ -10279,10 +10277,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 			      && modifier != EXPAND_STACK_PARM
 			      ? target : NULL_RTX),
 			     VOIDmode,
-			     (modifier == EXPAND_INITIALIZER
-			      || modifier == EXPAND_CONST_ADDRESS
-			      || modifier == EXPAND_STACK_PARM)
-			     ? modifier : EXPAND_NORMAL);
+			     modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
 
 	    if (MEM_P (orig_op0))
 	      {

-- 
Alan Modra
Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Richard Biener]

On Wed, Jun 12, 2013 at 4:48 AM, Alan Modra <amodra@gmail.com> wrote:
> The following patch fixes PR57134 by
> a) excluding bitfield expansion when EXPAND_MEMORY, and
> b) passing down the EXPAND_MEMORY modifier in a couple of places where
> this does not currently happen on recursive calls to expand_expr().
>
> (a) has precedent elsewhere in expr.c, eg. see expand_expr_real_1
> <MEM_REF>, (b) is a little difficult to justify except to claim
> there's no logical reason why it should be excluded nowadays.  Hand
> waving argument follows:
>
> Of the seven expand_expr() modifiers, recursive calls made to handle
> inner references of aggregate types currently exclude EXPAND_SUM,
> EXPAND_WRITE, and EXPAND_MEMORY from the modifier passed.  I suppose
> EXPAND_SUM is excluded so that the inner expand_expr() won't return a
> PLUS or MULT when we might be adding a further offset, but I can't see
> why the other two modifiers are not passed.  Digging through the
> history, it looks like EXPAND_WRITE may have been missed by accident,
> and EXPAND_MEMORY used to be an entirely different animal.  kenner was
> responsible for the original recursive call that passed down
> EXPAND_NORMAL, EXPAND_CONST_ADDRESS and EXPAND_INITIALIZER when expr.h
> looked like:
>
>  14612     kenner    EXPAND_MEMORY_USE_* are explained below.  */
>    406     kenner enum expand_modifier {EXPAND_NORMAL, EXPAND_SUM,
>  14612     kenner                     EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER,
>  14612     kenner                     EXPAND_MEMORY_USE_WO, EXPAND_MEMORY_USE_RW,
>  14612     kenner                     EXPAND_MEMORY_USE_BAD, EXPAND_MEMORY_USE_DONT};
>    406     kenner
>  14612     kenner /* Argument for chkr_* functions.
>  14612     kenner    MEMORY_USE_RO: the pointer reads memory.
>  14612     kenner    MEMORY_USE_WO: the pointer writes to memory.
>  14612     kenner    MEMORY_USE_RW: the pointer modifies memory (ie it reads and writes). An
>  14612     kenner                   example is (*ptr)++
>  14612     kenner    MEMORY_USE_BAD: use this if you don't know the behavior of the pointer, or
>  14612     kenner                    if you know there are no pointers.  Using an INDIRECT_REF
>  14612     kenner                    with MEMORY_USE_BAD will abort.
>  14612     kenner    MEMORY_USE_TW: just test for writing, without update.  Special.
>  14612     kenner    MEMORY_USE_DONT: the memory is neither read nor written.  This is used by
>  14612     kenner                  '->' and '.'.  */
>
> So I think passing EXPAND_MEMORY and EXPAND_WRITE down ought to be
> good.  The VIEW_CONVERT_EXPR case really doesn't need to exclude
> EXPAND_SUM either, since it doesn't allow an offset, but I made it the
> same as the inner ref case so that when/if someone attends to
>         /* ??? We should work harder and deal with non-zero offsets.  */
> it doesn't cause a surprise.  Really, a lot of this code needs
> attention, for example, I think SSA made EXPAND_STACK_PARM and all of
> the related code in calls.c dealing with libcalls when expanding args,
> unnecessary.
>
> Bootstrapped and regression tested powerpc64-linux.  OK to apply?

I suppose it also fixes PR57586 which looks similar?  Can you add a testcase
please?

Ok with a testcase and mentioning PR57586 in the changelog.

Thanks,
Richard.

>         PR middle-end/57134
>         * expr.c (expand_expr_real_1 <normal_inner_ref>): Pass
>         EXPAND_MEMORY and EXPAND_WRITE to recursive call.  Don't use
>         bitfield expansion when EXPAND_MEMORY.
>         (expand_expr_real_1 <VIEW_CONVERT_EXPR>): Pass modifier likewise.
>
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  (revision 199940)
> +++ gcc/expr.c  (working copy)
> @@ -9918,12 +9919,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
>                           && modifier != EXPAND_STACK_PARM
>                           ? target : NULL_RTX),
>                          VOIDmode,
> -                        (modifier == EXPAND_INITIALIZER
> -                         || modifier == EXPAND_CONST_ADDRESS
> -                         || modifier == EXPAND_STACK_PARM)
> -                        ? modifier : EXPAND_NORMAL);
> +                        modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>
> -
>         /* If the bitfield is volatile, we want to access it in the
>            field's mode, not the computed mode.
>            If a MEM has VOIDmode (external with incomplete type),
> @@ -10081,6 +10078,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
>                       || (MEM_P (op0)
>                           && (MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode1)
>                               || (bitpos % GET_MODE_ALIGNMENT (mode1) != 0))))
> +                    && modifier != EXPAND_MEMORY
>                      && ((modifier == EXPAND_CONST_ADDRESS
>                           || modifier == EXPAND_INITIALIZER)
>                          ? STRICT_ALIGNMENT
> @@ -10279,10 +10277,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
>                               && modifier != EXPAND_STACK_PARM
>                               ? target : NULL_RTX),
>                              VOIDmode,
> -                            (modifier == EXPAND_INITIALIZER
> -                             || modifier == EXPAND_CONST_ADDRESS
> -                             || modifier == EXPAND_STACK_PARM)
> -                            ? modifier : EXPAND_NORMAL);
> +                            modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>
>             if (MEM_P (orig_op0))
>               {
>
> --
> Alan Modra
> Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Alan Modra]

On Thu, Jun 13, 2013 at 10:45:38AM +0200, Richard Biener wrote:
> On Wed, Jun 12, 2013 at 4:48 AM, Alan Modra <amodra@gmail.com> wrote:
> > The following patch fixes PR57134 by
> > a) excluding bitfield expansion when EXPAND_MEMORY, and
> > b) passing down the EXPAND_MEMORY modifier in a couple of places where
> > this does not currently happen on recursive calls to expand_expr().
> 
> I suppose it also fixes PR57586 which looks similar?

Not completely.  It cures the ICE, but you still get "output number 0
not directly addressable".  The reason being that expand_asm_operands
isn't asking for a mem.  It should I guess, and also not specify
EXPAND_WRITE on an inout parameter.  Bootstrapped and regression
tested powerpc64-linux.

	PR middle-end/57586
	* stmt.c (expand_asm_operands): Call expand_expr with
	EXPAND_MEMORY for output operands that disallow regs.  Don't
	use EXPAND_WRITE on inout operands.

Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c	(revision 200083)
+++ gcc/stmt.c	(working copy)
@@ -807,7 +807,10 @@ expand_asm_operands (tree string, tree outputs, tr
 	  || ! allows_reg
 	  || is_inout)
 	{
-	  op = expand_expr (val, NULL_RTX, VOIDmode, EXPAND_WRITE);
+	  op = expand_expr (val, NULL_RTX, VOIDmode,
+			    !allows_reg ? EXPAND_MEMORY
+			    : !is_inout ? EXPAND_WRITE
+			    : EXPAND_NORMAL);
 	  if (MEM_P (op))
 	    op = validize_mem (op);
 

> Ok with a testcase and mentioning PR57586 in the changelog.

gcc.dg/compile/pr57134.c added.

-- 
Alan Modra
Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Richard Biener]

On Fri, Jun 14, 2013 at 10:38 AM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 10:45:38AM +0200, Richard Biener wrote:
>> On Wed, Jun 12, 2013 at 4:48 AM, Alan Modra <amodra@gmail.com> wrote:
>> > The following patch fixes PR57134 by
>> > a) excluding bitfield expansion when EXPAND_MEMORY, and
>> > b) passing down the EXPAND_MEMORY modifier in a couple of places where
>> > this does not currently happen on recursive calls to expand_expr().
>>
>> I suppose it also fixes PR57586 which looks similar?
>
> Not completely.  It cures the ICE, but you still get "output number 0
> not directly addressable".  The reason being that expand_asm_operands
> isn't asking for a mem.  It should I guess, and also not specify
> EXPAND_WRITE on an inout parameter.  Bootstrapped and regression
> tested powerpc64-linux.

It looks reasonable to me, but I'm not too familiar with EXPAND_MEMORY
vs. EXPAND_WRITE.

Btw, I wonder if for strict-alignment targets asm()s can expect "aligned"
memory if they request an asm input with "m"?  Thus, do we eventually
have to copy a known unaligned mem to aligned scratch memory before
passing it to a "m" input?  Do we maybe have to do the same even for
"m" outputs?  Or is this all simply undefined and asm()s have to handle
arbitrary alignment of memory operands (well, those that appear
at runtime, of course).

Richard.

>         PR middle-end/57586
>         * stmt.c (expand_asm_operands): Call expand_expr with
>         EXPAND_MEMORY for output operands that disallow regs.  Don't
>         use EXPAND_WRITE on inout operands.
>
> Index: gcc/stmt.c
> ===================================================================
> --- gcc/stmt.c  (revision 200083)
> +++ gcc/stmt.c  (working copy)
> @@ -807,7 +807,10 @@ expand_asm_operands (tree string, tree outputs, tr
>           || ! allows_reg
>           || is_inout)
>         {
> -         op = expand_expr (val, NULL_RTX, VOIDmode, EXPAND_WRITE);
> +         op = expand_expr (val, NULL_RTX, VOIDmode,
> +                           !allows_reg ? EXPAND_MEMORY
> +                           : !is_inout ? EXPAND_WRITE
> +                           : EXPAND_NORMAL);
>           if (MEM_P (op))
>             op = validize_mem (op);
>
>
>> Ok with a testcase and mentioning PR57586 in the changelog.
>
> gcc.dg/compile/pr57134.c added.
>
> --
> Alan Modra
> Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Alan Modra]

This is a followup to
http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00837.html
which is still lacking an OK.  Apologies for dropping this patch on
the floor.

	PR middle-end/57586
	* stmt.c (expand_asm_operands): Call expand_expr with
	EXPAND_MEMORY for output operands that disallow regs.  Don't
	use EXPAND_WRITE on inout operands.

Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c	(revision 202428)
+++ gcc/stmt.c	(working copy)
@@ -806,7 +806,10 @@ expand_asm_operands (tree string, tree outputs, tr
 	  || ! allows_reg
 	  || is_inout)
 	{
-	  op = expand_expr (val, NULL_RTX, VOIDmode, EXPAND_WRITE);
+	  op = expand_expr (val, NULL_RTX, VOIDmode,
+			    !allows_reg ? EXPAND_MEMORY
+			    : !is_inout ? EXPAND_WRITE
+			    : EXPAND_NORMAL);
 	  if (MEM_P (op))
 	    op = validize_mem (op);
 

On Fri, Jun 14, 2013 at 11:38:58AM +0200, Richard Biener wrote:
> On Fri, Jun 14, 2013 at 10:38 AM, Alan Modra <amodra@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 10:45:38AM +0200, Richard Biener wrote:
> >> On Wed, Jun 12, 2013 at 4:48 AM, Alan Modra <amodra@gmail.com> wrote:
> >> > The following patch fixes PR57134 by
> >> > a) excluding bitfield expansion when EXPAND_MEMORY, and
> >> > b) passing down the EXPAND_MEMORY modifier in a couple of places where
> >> > this does not currently happen on recursive calls to expand_expr().
> >>
> >> I suppose it also fixes PR57586 which looks similar?
> >
> > Not completely.  It cures the ICE, but you still get "output number 0
> > not directly addressable".  The reason being that expand_asm_operands
> > isn't asking for a mem.  It should I guess, and also not specify
> > EXPAND_WRITE on an inout parameter.  Bootstrapped and regression
> > tested powerpc64-linux.
> 
> It looks reasonable to me, but I'm not too familiar with EXPAND_MEMORY
> vs. EXPAND_WRITE.

For the expr.h comment
"EXPAND_WRITE means we are only going to write to the resulting rtx."
So fairly obviously we shouldn't use that with inout asm args.

> Btw, I wonder if for strict-alignment targets asm()s can expect "aligned"
> memory if they request an asm input with "m"?  Thus, do we eventually
> have to copy a known unaligned mem to aligned scratch memory before
> passing it to a "m" input?  Do we maybe have to do the same even for
> "m" outputs?  Or is this all simply undefined and asm()s have to handle
> arbitrary alignment of memory operands (well, those that appear
> at runtime, of course).

I'm sure the kernel people would rather *not* have copies to scratch
memory.  The testcase in pr57586 was derived from kernel code that
munges pointers.  A testcase that better shows what is going on,
probably from the same kernel code, is here
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57134#c3
(The pointer munging is what causes gcc to lose track of alignment.)

-- 
Alan Modra
Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Alan Modra]

On Fri, Sep 13, 2013 at 12:37:20PM +0930, Alan Modra wrote:
> 	PR middle-end/57586
> 	* stmt.c (expand_asm_operands): Call expand_expr with
> 	EXPAND_MEMORY for output operands that disallow regs.  Don't
> 	use EXPAND_WRITE on inout operands.

Ping?

-- 
Alan Modra
Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Richard Biener]

On Tue, Sep 24, 2013 at 12:43 PM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Sep 13, 2013 at 12:37:20PM +0930, Alan Modra wrote:
>>       PR middle-end/57586
>>       * stmt.c (expand_asm_operands): Call expand_expr with
>>       EXPAND_MEMORY for output operands that disallow regs.  Don't
>>       use EXPAND_WRITE on inout operands.
>
> Ping?

Ok.

Thanks,
Richard.

> --
> Alan Modra
> Australia Development Lab, IBM

Re: expand_expr tweaks to fix PR57134

[Alan Modra]

I'm committing this cleanup patch to my PR 57134,57586 changes as
obvious.  That it is obvious can be seen from an assert in
tree-ssa-operands.c get_asm_expr_operands().

      /* This should have been split in gimplify_asm_expr.  */
      gcc_assert (!allows_reg || !is_inout);

Bootstrapped, etc. powerpc64-linux.

	* stmt.c (expand_asm_operands): Revert part of 2013-09-24 special
	casing inout operands.

Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c	(revision 203053)
+++ gcc/stmt.c	(working copy)
@@ -807,9 +807,7 @@ expand_asm_operands (tree string, tree outputs, tr
 	  || is_inout)
 	{
 	  op = expand_expr (val, NULL_RTX, VOIDmode,
-			    !allows_reg ? EXPAND_MEMORY
-			    : !is_inout ? EXPAND_WRITE
-			    : EXPAND_NORMAL);
+			    !allows_reg ? EXPAND_MEMORY : EXPAND_WRITE);
 	  if (MEM_P (op))
 	    op = validize_mem (op);
 

-- 
Alan Modra
Australia Development Lab, IBM