| Message ID | 1516714305-20905-1-git-send-email-blomqvist.janne@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR 78534 Reinstate better string copy algorithm | expand |
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 >
OK. Thought I already sent OK, but must of got side track with work.
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" } }