| Message ID | 87vag1wzk5.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | VIEW_CONVERT_EXPR slots for strict-align targets (PR 83884) | expand |
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
> 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).
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
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.