PING
On Tue, Jan 23, 2018 at 3:31 PM, Janne Blomqvist
<blomqvist.ja...@gmail.com> wrote:
> As part of the change to larger character lengths, the string copy
> algorithm was temporarily pessimized to get around some spurious
> -Wstringop-overflow warnings. Having tried a number of variations of
> this algorithm I have managed to get it down to one spurious warning,
> only with -O1 optimization, in the testsuite. This patch reinstates
> the optimized variant and modifies this one testcase to ignore the
> warning.
>
> Regtested on x86_64-pc-linux-gnu, Ok for trunk?
>
> gcc/fortran/ChangeLog:
>
> 2018-01-23 Janne Blomqvist <j...@gcc.gnu.org>
>
> PR 78534
> * trans-expr.c (fill_with_spaces): Use memset instead of
> generating loop.
> (gfc_trans_string_copy): Improve opportunity to use builtins with
> constant lengths.
>
> gcc/testsuite/ChangeLog:
>
> 2018-01-23 Janne Blomqvist <j...@gcc.gnu.org>
>
> PR 78534
> * gfortran.dg/allocate_deferred_char_scalar_1.f03: Prune
> -Wstringop-overflow warnings due to spurious warning with -O1.
> * gfortran.dg/char_cast_1.f90: Update dump scan pattern.
> * gfortran.dg/transfer_intrinsic_1.f90: Likewise.
> ---
> gcc/fortran/trans-expr.c | 52
> ++++++++++++----------
> .../allocate_deferred_char_scalar_1.f03 | 2 +
> gcc/testsuite/gfortran.dg/char_cast_1.f90 | 6 +--
> gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90 | 2 +-
> 4 files changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
> index e90036f..4da6cee 100644
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -6407,8 +6407,6 @@ fill_with_spaces (tree start, tree type, tree size)
> tree i, el, exit_label, cond, tmp;
>
> /* For a simple char type, we can call memset(). */
> - /* TODO: This code does work and is potentially more efficient, but
> - causes spurious -Wstringop-overflow warnings.
> if (compare_tree_int (TYPE_SIZE_UNIT (type), 1) == 0)
> return build_call_expr_loc (input_location,
> builtin_decl_explicit (BUILT_IN_MEMSET),
> @@ -6416,7 +6414,6 @@ fill_with_spaces (tree start, tree type, tree size)
> build_int_cst (gfc_get_int_type (gfc_c_int_kind),
> lang_hooks.to_target_charset ('
> ')),
> fold_convert (size_type_node, size));
> - */
>
> /* Otherwise, we use a loop:
> for (el = start, i = size; i > 0; el--, i+= TYPE_SIZE_UNIT (type))
> @@ -6522,11 +6519,20 @@ gfc_trans_string_copy (stmtblock_t * block, tree
> dlength, tree dest,
>
> /* The string copy algorithm below generates code like
>
> - if (dlen > 0) {
> - memmove (dest, src, min(dlen, slen));
> - if (slen < dlen)
> - memset(&dest[slen], ' ', dlen - slen);
> - }
> + if (destlen > 0)
> + {
> + if (srclen < destlen)
> + {
> + memmove (dest, src, srclen);
> + // Pad with spaces.
> + memset (&dest[srclen], ' ', destlen - srclen);
> + }
> + else
> + {
> + // Truncate if too long.
> + memmove (dest, src, destlen);
> + }
> + }
> */
>
> /* Do nothing if the destination length is zero. */
> @@ -6555,21 +6561,16 @@ gfc_trans_string_copy (stmtblock_t * block, tree
> dlength, tree dest,
> else
> src = gfc_build_addr_expr (pvoid_type_node, src);
>
> - /* First do the memmove. */
> - tmp2 = fold_build2_loc (input_location, MIN_EXPR, TREE_TYPE (dlen), dlen,
> - slen);
> - tmp2 = build_call_expr_loc (input_location,
> - builtin_decl_explicit (BUILT_IN_MEMMOVE),
> - 3, dest, src,
> - fold_convert (size_type_node, tmp2));
> - stmtblock_t tmpblock2;
> - gfc_init_block (&tmpblock2);
> - gfc_add_expr_to_block (&tmpblock2, tmp2);
> -
> - /* If the destination is longer, fill the end with spaces. */
> + /* Truncate string if source is too long. */
> cond2 = fold_build2_loc (input_location, LT_EXPR, logical_type_node, slen,
> dlen);
>
> + /* Copy and pad with spaces. */
> + tmp3 = build_call_expr_loc (input_location,
> + builtin_decl_explicit (BUILT_IN_MEMMOVE),
> + 3, dest, src,
> + fold_convert (size_type_node, slen));
> +
> /* Wstringop-overflow appears at -O3 even though this warning is not
> explicitly available in fortran nor can it be switched off. If the
> source length is a constant, its negative appears as a very large
> @@ -6584,14 +6585,19 @@ gfc_trans_string_copy (stmtblock_t * block, tree
> dlength, tree dest,
> tmp4 = fill_with_spaces (tmp4, chartype, tmp);
>
> gfc_init_block (&tempblock);
> + gfc_add_expr_to_block (&tempblock, tmp3);
> gfc_add_expr_to_block (&tempblock, tmp4);
> tmp3 = gfc_finish_block (&tempblock);
>
> + /* The truncated memmove if the slen >= dlen. */
> + tmp2 = build_call_expr_loc (input_location,
> + builtin_decl_explicit (BUILT_IN_MEMMOVE),
> + 3, dest, src,
> + fold_convert (size_type_node, dlen));
> +
> /* The whole copy_string function is there. */
> tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, cond2,
> - tmp3, build_empty_stmt (input_location));
> - gfc_add_expr_to_block (&tmpblock2, tmp);
> - tmp = gfc_finish_block (&tmpblock2);
> + tmp3, tmp2);
> tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, cond,
> tmp,
> build_empty_stmt (input_location));
> gfc_add_expr_to_block (block, tmp);
> diff --git a/gcc/testsuite/gfortran.dg/allocate_deferred_char_scalar_1.f03
> b/gcc/testsuite/gfortran.dg/allocate_deferred_char_scalar_1.f03
> index b9b7040..d5ca573 100644
> --- a/gcc/testsuite/gfortran.dg/allocate_deferred_char_scalar_1.f03
> +++ b/gcc/testsuite/gfortran.dg/allocate_deferred_char_scalar_1.f03
> @@ -265,3 +265,5 @@ contains
> if(len(p4) /= 3) call abort()
> end subroutine source3
> end program test
> +! Spurious -Wstringop-overflow warning with -O1
> +! { dg-prune-output "\\\[-Wstringop-overflow=]" }
> diff --git a/gcc/testsuite/gfortran.dg/char_cast_1.f90
> b/gcc/testsuite/gfortran.dg/char_cast_1.f90
> index 70963bb..02e695d 100644
> --- a/gcc/testsuite/gfortran.dg/char_cast_1.f90
> +++ b/gcc/testsuite/gfortran.dg/char_cast_1.f90
> @@ -25,6 +25,6 @@
> return
> end function Upper
> end
> -! The sign that all is well is that [S.10][1] appears twice.
> -! Platform dependent variations are [S$10][1], [__S_10][1], [S___10][1]
> -! { dg-final { scan-tree-dump-times "10\\\]\\\[1\\\]" 2 "original" } }
> +! The sign that all is well is that [S.6][1] appears twice.
> +! Platform dependent variations are [S$6][1], [__S_6][1], [S___6][1]
> +! { dg-final { scan-tree-dump-times "6\\\]\\\[1\\\]" 2 "original" } }
> diff --git a/gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90
> b/gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90
> index 73a7e77..5f46cd0 100644
> --- a/gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90
> +++ b/gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90
> @@ -14,4 +14,4 @@ subroutine BytesToString(bytes, string)
> character(len=*) :: string
> string = transfer(bytes, string)
> end subroutine
> -! { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "original" } }
> --
> 2.7.4
>
--
Janne Blomqvist
