VIEW_CONVERT_EXPR slots for strict-align targets (PR 83884)

Message ID 87vag1wzk5.fsf@linaro.org
State New
Headers show
Series
  • VIEW_CONVERT_EXPR slots for strict-align targets (PR 83884)
Related show

Commit Message

Richard Sandiford Jan. 16, 2018, 4:14 p.m.
This PR is about a case in which we VIEW_CONVERT a variable-sized
unaligned record:

 <record_type 0x7ffff6d92888 check_displace_generation__T245b sizes-gimplified type_7 BLK
    size <var_decl 0x7ffff6846510 D.3499 ...>
    unit-size <var_decl 0x7ffff68465a0 D.3500 ...>
    align:8 ...>

to an aligned 32-bit integer.  The strict-alignment handling of
this case creates an aligned temporary slot, moves the operand
into the slot in the operand's original mode, then accesses the
slot in the more-aligned result mode.

Previously the size of the temporary slot was calculated using:

                  HOST_WIDE_INT temp_size
                    = MAX (int_size_in_bytes (inner_type),
                           (HOST_WIDE_INT) GET_MODE_SIZE (mode));

int_size_in_bytes would return -1 for the variable-length type,
so we'd use the size of the result mode for the slot.  r256152 replaced
int_size_in_bytes with tree_to_poly_uint64, which triggered an ICE.

I'd assumed that variable-length types couldn't occur here, since it
seems strange to view-convert a variable-length type to a fixed-length
one.  It also seemed strange that (with the old code) we'd ignore the
size of the operand if it was a variable V but honour it if it was a
constant C, even though it's presumably possible for V to equal that
C at runtime.

If op0 has BLKmode we do a block copy of GET_MODE_SIZE (mode) bytes
and then convert the slot to "mode":

		  poly_uint64 mode_size = GET_MODE_SIZE (mode);
		  ...
		  if (GET_MODE (op0) == BLKmode)
		    {
		      rtx size_rtx = gen_int_mode (mode_size, Pmode);
		      emit_block_move (new_with_op0_mode, op0, size_rtx,
				       (modifier == EXPAND_STACK_PARM
					? BLOCK_OP_CALL_PARM
					: BLOCK_OP_NORMAL));
		    }
		  else
		    ...

		  op0 = new_rtx;
		}
	    }

	  op0 = adjust_address (op0, mode, 0);

so I think in that case just the size of "mode" is enough, even if op0
is a fixed-size type.  For non-BLKmode op0 we first move in op0's mode
and then convert the slot to "mode":

		    emit_move_insn (new_with_op0_mode, op0);

		  op0 = new_rtx;
		}
	    }

	  op0 = adjust_address (op0, mode, 0);

so I think we want the maximum of the two mode sizes in that case
(assuming they can be different sizes).

But is this VIEW_CONVERT_EXPR really valid?  Maybe this is just
papering over a deeper issue.  There again, the MAX in the old
code was presumably there because the sizes can be different...

Richard


2018-01-16  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR middle-end/83884
	* expr.c (expand_expr_real_1): Use the size of GET_MODE (op0)
	rather than the size of inner_type to determine the stack slot size
	when handling VIEW_CONVERT_EXPRs on strict-alignment targets.

Comments

Richard Biener Jan. 16, 2018, 5:59 p.m. | #1
On January 16, 2018 5:14:50 PM GMT+01:00, Richard Sandiford <richard.sandiford@linaro.org> wrote:
>This PR is about a case in which we VIEW_CONVERT a variable-sized

>unaligned record:

>

><record_type 0x7ffff6d92888 check_displace_generation__T245b

>sizes-gimplified type_7 BLK

>    size <var_decl 0x7ffff6846510 D.3499 ...>

>    unit-size <var_decl 0x7ffff68465a0 D.3500 ...>

>    align:8 ...>

>

>to an aligned 32-bit integer.  The strict-alignment handling of

>this case creates an aligned temporary slot, moves the operand

>into the slot in the operand's original mode, then accesses the

>slot in the more-aligned result mode.

>

>Previously the size of the temporary slot was calculated using:

>

>                  HOST_WIDE_INT temp_size

>                    = MAX (int_size_in_bytes (inner_type),

>                           (HOST_WIDE_INT) GET_MODE_SIZE (mode));

>

>int_size_in_bytes would return -1 for the variable-length type,

>so we'd use the size of the result mode for the slot.  r256152 replaced

>int_size_in_bytes with tree_to_poly_uint64, which triggered an ICE.

>

>I'd assumed that variable-length types couldn't occur here, since it

>seems strange to view-convert a variable-length type to a fixed-length

>one.  It also seemed strange that (with the old code) we'd ignore the

>size of the operand if it was a variable V but honour it if it was a

>constant C, even though it's presumably possible for V to equal that

>C at runtime.

>

>If op0 has BLKmode we do a block copy of GET_MODE_SIZE (mode) bytes

>and then convert the slot to "mode":

>

>		  poly_uint64 mode_size = GET_MODE_SIZE (mode);

>		  ...

>		  if (GET_MODE (op0) == BLKmode)

>		    {

>		      rtx size_rtx = gen_int_mode (mode_size, Pmode);

>		      emit_block_move (new_with_op0_mode, op0, size_rtx,

>				       (modifier == EXPAND_STACK_PARM

>					? BLOCK_OP_CALL_PARM

>					: BLOCK_OP_NORMAL));

>		    }

>		  else

>		    ...

>

>		  op0 = new_rtx;

>		}

>	    }

>

>	  op0 = adjust_address (op0, mode, 0);

>

>so I think in that case just the size of "mode" is enough, even if op0

>is a fixed-size type.  For non-BLKmode op0 we first move in op0's mode

>and then convert the slot to "mode":

>

>		    emit_move_insn (new_with_op0_mode, op0);

>

>		  op0 = new_rtx;

>		}

>	    }

>

>	  op0 = adjust_address (op0, mode, 0);

>

>so I think we want the maximum of the two mode sizes in that case

>(assuming they can be different sizes).

>

>But is this VIEW_CONVERT_EXPR really valid?  


IMHO it is on the border of be being invalid (verify_gimple doesn't diagnose it). Using a BIT_FIELD_REF would be much better here. 

Richard. 

Maybe this is just
>papering over a deeper issue.  There again, the MAX in the old

>code was presumably there because the sizes can be different...

>

>Richard

>

>

>2018-01-16  Richard Sandiford  <richard.sandiford@linaro.org>

>

>gcc/

>	PR middle-end/83884

>	* expr.c (expand_expr_real_1): Use the size of GET_MODE (op0)

>	rather than the size of inner_type to determine the stack slot size

>	when handling VIEW_CONVERT_EXPRs on strict-alignment targets.

>

>Index: gcc/expr.c

>===================================================================

>--- gcc/expr.c	2018-01-14 08:42:44.497155977 +0000

>+++ gcc/expr.c	2018-01-16 16:07:22.737883774 +0000

>@@ -11145,11 +11145,11 @@ expand_expr_real_1 (tree exp, rtx target

> 		}

> 	      else if (STRICT_ALIGNMENT)

> 		{

>-		  tree inner_type = TREE_TYPE (treeop0);

> 		  poly_uint64 mode_size = GET_MODE_SIZE (mode);

>-		  poly_uint64 op0_size

>-		    = tree_to_poly_uint64 (TYPE_SIZE_UNIT (inner_type));

>-		  poly_int64 temp_size = upper_bound (op0_size, mode_size);

>+		  poly_uint64 temp_size = mode_size;

>+		  if (GET_MODE (op0) != BLKmode)

>+		    temp_size = upper_bound (temp_size,

>+					     GET_MODE_SIZE (GET_MODE (op0)));

> 		  rtx new_rtx

> 		    = assign_stack_temp_for_type (mode, temp_size, type);

> 		  rtx new_with_op0_mode
Eric Botcazou Jan. 16, 2018, 8:24 p.m. | #2
> I'd assumed that variable-length types couldn't occur here, since it

> seems strange to view-convert a variable-length type to a fixed-length

> one.


This happens all the time in Ada when you convert an unconstrained type into 
one of its constrained subtypes (but the run-time sizes must match).

> But is this VIEW_CONVERT_EXPR really valid?  Maybe this is just

> papering over a deeper issue.  There again, the MAX in the old

> code was presumably there because the sizes can be different...


The problem is that Ada exposes VIEW_CONVERT_EXPR to the user and the user can 
do very weird things with it so you need to be prepared for the worst.

> 2018-01-16  Richard Sandiford  <richard.sandiford@linaro.org>

> 

> gcc/

> 	PR middle-end/83884

> 	* expr.c (expand_expr_real_1): Use the size of GET_MODE (op0)

> 	rather than the size of inner_type to determine the stack slot size

> 	when handling VIEW_CONVERT_EXPRs on strict-alignment targets.


This looks good to me, thanks for fixing the problem.  Unexpectedly enough, I 
don't see the failures on SPARC (32-bit or 64-bit).

-- 
Eric Botcazou

Patch

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	2018-01-14 08:42:44.497155977 +0000
+++ gcc/expr.c	2018-01-16 16:07:22.737883774 +0000
@@ -11145,11 +11145,11 @@  expand_expr_real_1 (tree exp, rtx target
 		}
 	      else if (STRICT_ALIGNMENT)
 		{
-		  tree inner_type = TREE_TYPE (treeop0);
 		  poly_uint64 mode_size = GET_MODE_SIZE (mode);
-		  poly_uint64 op0_size
-		    = tree_to_poly_uint64 (TYPE_SIZE_UNIT (inner_type));
-		  poly_int64 temp_size = upper_bound (op0_size, mode_size);
+		  poly_uint64 temp_size = mode_size;
+		  if (GET_MODE (op0) != BLKmode)
+		    temp_size = upper_bound (temp_size,
+					     GET_MODE_SIZE (GET_MODE (op0)));
 		  rtx new_rtx
 		    = assign_stack_temp_for_type (mode, temp_size, type);
 		  rtx new_with_op0_mode