[PATCH] PR 78534 Reinstate better string copy algorithm
Steve Kargl
sgk@troutmask.apl.washington.edu
Tue Jan 30 22:46:00 GMT 2018
OK. Thought I already sent OK, but must of got side track with work.
--
steve
On Tue, Jan 30, 2018 at 06:25:31PM +0200, Janne Blomqvist wrote:
> PING
>
> On Tue, Jan 23, 2018 at 3:31 PM, Janne Blomqvist
> <blomqvist.janne@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 <jb@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 <jb@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
--
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow
More information about the Gcc-patches
mailing list