Re: [PATCH v2] Fortran: Narrow return types [PR78798]
On Sun, 14 May 2023 14:27:42 +0200
Mikael Morin wrote:
> Le 10/05/2023 à 18:47, Bernhard Reutner-Fischer via Fortran a écrit :
> > From: Bernhard Reutner-Fischer
> >
> > gcc/fortran/ChangeLog:
> >
> > PR fortran/78798
> > * array.cc (compare_bounds): Use narrower return type.
> > (gfc_compare_array_spec): Likewise.
> > (is_constant_element): Likewise.
> > (gfc_constant_ac): Likewise.
> (...)
> > ---
> > Bootstrapped without new warnings and regression tested on
> > x86_64-linux with no regressions, OK for trunk?
> >
> (...)
> > diff --git a/gcc/fortran/check.cc b/gcc/fortran/check.cc
> > index b348bda6e6c..4e3aed84b9d 100644
> > --- a/gcc/fortran/check.cc
> > +++ b/gcc/fortran/check.cc
> > @@ -1156,7 +1156,7 @@ dim_rank_check (gfc_expr *dim, gfc_expr *array, int
> > allow_assumed)
> > dimension bi, returning 0 if they are known not to be identical,
> > and 1 if they are identical, or if this cannot be determined. */
> >
> > -static int
> > +static bool
> > identical_dimen_shape (gfc_expr *a, int ai, gfc_expr *b, int bi)
> > {
> > mpz_t a_size, b_size;
>
> To be consistent, please change as well the local variable "ret" used as
> return value from int to bool.
>
> > diff --git a/gcc/fortran/cpp.cc b/gcc/fortran/cpp.cc
> > index c3b7c7f7bd9..d7890a97287 100644
> > --- a/gcc/fortran/cpp.cc
> > +++ b/gcc/fortran/cpp.cc
> > @@ -297,7 +297,7 @@ gfc_cpp_init_options (unsigned int
> > decoded_options_count,
> > gfc_cpp_option.deferred_opt_count = 0;
> > }
> >
> > -int
> > +bool
> > gfc_cpp_handle_option (size_t scode, const char *arg, int value
> > ATTRIBUTE_UNUSED)
> > {
> > int result = 1;
>
> Same here, change the type of variable "result".
>
> (...)
> > diff --git a/gcc/fortran/dependency.cc b/gcc/fortran/dependency.cc
> > index a648d5c7903..b398b29a642 100644
> > --- a/gcc/fortran/dependency.cc
> > +++ b/gcc/fortran/dependency.cc
> (...)
>
> > @@ -1091,7 +1091,7 @@ gfc_check_argument_dependency (gfc_expr *other,
> > sym_intent intent,
> > /* Like gfc_check_argument_dependency, but check all the arguments in
> > ACTUAL.
> > FNSYM is the function being called, or NULL if not known. */
> >
> > -int
> > +bool
> > gfc_check_fncall_dependency (gfc_expr *other, sym_intent intent,
> > gfc_symbol *fnsym, gfc_actual_arglist *actual,
> > gfc_dep_check elemental)
>
> Why not change the associated subfunctions
> (gfc_check_argument_dependency, gfc_check_argument_var_dependency) as well ?
I have left these subfunctions alone for now to get the other hunks out
of the way. I have adjusted the patch according to your other comments
and pushed it as r14-973-gc072df1ab14450.
Thanks!
>
> (...)
> > @@ -2098,7 +2098,7 @@ ref_same_as_full_array (gfc_ref *full_ref, gfc_ref
> > *ref)
> > there is some kind of overlap.
> > 0 : array references are identical or not overlapping. */
> >
> > -int
> > +bool
> > gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
> > bool identical)
> > {
>
> The function comment states that the function may return 2, which
> doesn't seem to be the case any more. So please update the comment.
>
> (...)> diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc
> > index 221165d6dac..b4b36e27d75 100644
> > --- a/gcc/fortran/symbol.cc
> > +++ b/gcc/fortran/symbol.cc
> > @@ -3216,7 +3216,7 @@ gfc_find_symtree_in_proc (const char* name,
> > gfc_namespace* ns)
> > any parent namespaces if requested by a nonzero parent_flag.
> > Returns nonzero if the name is ambiguous. */
> >
> > -int
> > +bool
> > gfc_find_sym_tree (const char *name, gfc_namespace *ns, int parent_flag,
> >gfc_symtree **result)
> > {
>
> Maybe change nonzero to true in the comment?
>
> (...)
>
> OK with all the above fixed.
>
> Thanks.
>
Re: [PATCH v2] Fortran: Narrow return types [PR78798]
Le 14/05/2023 à 17:24, Bernhard Reutner-Fischer a écrit :
On Sun, 14 May 2023 15:04:15 +0200
Thomas Koenig wrote:
On 14.05.23 14:27, Mikael Morin wrote:
(...)
@@ -2098,7 +2098,7 @@ ref_same_as_full_array (gfc_ref *full_ref,
gfc_ref *ref)
there is some kind of overlap.
0 : array references are identical or not overlapping. */
-int
+bool
gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
bool identical)
{
The function comment states that the function may return 2, which
doesn't seem to be the case any more.
Hm, this makes me a litte suspicious. Was functionality for reversing
loops lost, maybe unintentionally? I assume that, at some time, we did
use the '2' as return value (or did we?)
There was 7c428aa29d75ef163c334cf3974f87b3630d8b8b (a revert because it
miscompiled spec2k) which might have associated the comment of the
former static gfc_dependency dep_ref (gfc_ref *lref, gfc_ref *rref,
gfc_reverse *reverse) to the current gfc_dep_resolver.
The commit which introduced the return value 2 documentation was
3d03ead0b8273efde57f6194617b35111a84b05d
"re PR fortran/24524 (Fortran dependency checking should reverse loops)"
but TBH i don't see how it returned 2 in that revision?
Looks like when writing that patch it deemed useful to return 2 for
this specific situation but in the end it was dropped but the comment
survived.
Yes, I came to the same conclusion that we never returned 2 here.
The information that reversal is needed is already provided on a per
dimension basis by the gfc_reverse pointer passed as argument, so
providing the information in the return value would be redundant anyway.
I will update the comment to document the true / false return values.
And Mikael, do you want me to cleanup 1/0 to true/false assignments for
the boolean variables, or can we do that in a separate patch (or not at
all right now)?
I don't mind too much either way.
As long as the variables are not assigned integer values outside of the
[0,1] range, and we consistently use true/false or 0/1, not a mix of
them, it's fine with me.
Re: [PATCH v2] Fortran: Narrow return types [PR78798]
On Sun, 14 May 2023 15:04:15 +0200
Thomas Koenig wrote:
> On 14.05.23 14:27, Mikael Morin wrote:
> >
> > (...)
> >> @@ -2098,7 +2098,7 @@ ref_same_as_full_array (gfc_ref *full_ref,
> >> gfc_ref *ref)
> >> there is some kind of overlap.
> >> 0 : array references are identical or not overlapping. */
> >> -int
> >> +bool
> >> gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
> >> bool identical)
> >> {
> >
> > The function comment states that the function may return 2, which
> > doesn't seem to be the case any more.
>
> Hm, this makes me a litte suspicious. Was functionality for reversing
> loops lost, maybe unintentionally? I assume that, at some time, we did
> use the '2' as return value (or did we?)
There was 7c428aa29d75ef163c334cf3974f87b3630d8b8b (a revert because it
miscompiled spec2k) which might have associated the comment of the
former static gfc_dependency dep_ref (gfc_ref *lref, gfc_ref *rref,
gfc_reverse *reverse) to the current gfc_dep_resolver.
The commit which introduced the return value 2 documentation was
3d03ead0b8273efde57f6194617b35111a84b05d
"re PR fortran/24524 (Fortran dependency checking should reverse loops)"
but TBH i don't see how it returned 2 in that revision?
Looks like when writing that patch it deemed useful to return 2 for
this specific situation but in the end it was dropped but the comment
survived.
I will update the comment to document the true / false return values.
And Mikael, do you want me to cleanup 1/0 to true/false assignments for
the boolean variables, or can we do that in a separate patch (or not at
all right now)?
many thanks for the reviews!
Re: [PATCH v2] Fortran: Narrow return types [PR78798]
On 14.05.23 14:27, Mikael Morin wrote:
(...)
@@ -2098,7 +2098,7 @@ ref_same_as_full_array (gfc_ref *full_ref,
gfc_ref *ref)
there is some kind of overlap.
0 : array references are identical or not overlapping. */
-int
+bool
gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
bool identical)
{
The function comment states that the function may return 2, which
doesn't seem to be the case any more.
Hm, this makes me a litte suspicious. Was functionality for reversing
loops lost, maybe unintentionally? I assume that, at some time, we did
use the '2' as return value (or did we?)
Best regards
Thomas
Re: [PATCH v2] Fortran: Narrow return types [PR78798]
Le 10/05/2023 à 18:47, Bernhard Reutner-Fischer via Fortran a écrit :
From: Bernhard Reutner-Fischer
gcc/fortran/ChangeLog:
PR fortran/78798
* array.cc (compare_bounds): Use narrower return type.
(gfc_compare_array_spec): Likewise.
(is_constant_element): Likewise.
(gfc_constant_ac): Likewise.
(...)
---
Bootstrapped without new warnings and regression tested on
x86_64-linux with no regressions, OK for trunk?
(...)
diff --git a/gcc/fortran/check.cc b/gcc/fortran/check.cc
index b348bda6e6c..4e3aed84b9d 100644
--- a/gcc/fortran/check.cc
+++ b/gcc/fortran/check.cc
@@ -1156,7 +1156,7 @@ dim_rank_check (gfc_expr *dim, gfc_expr *array, int
allow_assumed)
dimension bi, returning 0 if they are known not to be identical,
and 1 if they are identical, or if this cannot be determined. */
-static int
+static bool
identical_dimen_shape (gfc_expr *a, int ai, gfc_expr *b, int bi)
{
mpz_t a_size, b_size;
To be consistent, please change as well the local variable "ret" used as
return value from int to bool.
diff --git a/gcc/fortran/cpp.cc b/gcc/fortran/cpp.cc
index c3b7c7f7bd9..d7890a97287 100644
--- a/gcc/fortran/cpp.cc
+++ b/gcc/fortran/cpp.cc
@@ -297,7 +297,7 @@ gfc_cpp_init_options (unsigned int decoded_options_count,
gfc_cpp_option.deferred_opt_count = 0;
}
-int
+bool
gfc_cpp_handle_option (size_t scode, const char *arg, int value
ATTRIBUTE_UNUSED)
{
int result = 1;
Same here, change the type of variable "result".
(...)
diff --git a/gcc/fortran/dependency.cc b/gcc/fortran/dependency.cc
index a648d5c7903..b398b29a642 100644
--- a/gcc/fortran/dependency.cc
+++ b/gcc/fortran/dependency.cc
(...)
@@ -1091,7 +1091,7 @@ gfc_check_argument_dependency (gfc_expr *other,
sym_intent intent,
/* Like gfc_check_argument_dependency, but check all the arguments in ACTUAL.
FNSYM is the function being called, or NULL if not known. */
-int
+bool
gfc_check_fncall_dependency (gfc_expr *other, sym_intent intent,
gfc_symbol *fnsym, gfc_actual_arglist *actual,
gfc_dep_check elemental)
Why not change the associated subfunctions
(gfc_check_argument_dependency, gfc_check_argument_var_dependency) as well ?
(...)
@@ -2098,7 +2098,7 @@ ref_same_as_full_array (gfc_ref *full_ref, gfc_ref *ref)
there is some kind of overlap.
0 : array references are identical or not overlapping. */
-int
+bool
gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
bool identical)
{
The function comment states that the function may return 2, which
doesn't seem to be the case any more. So please update the comment.
(...)> diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc
index 221165d6dac..b4b36e27d75 100644
--- a/gcc/fortran/symbol.cc
+++ b/gcc/fortran/symbol.cc
@@ -3216,7 +3216,7 @@ gfc_find_symtree_in_proc (const char* name,
gfc_namespace* ns)
any parent namespaces if requested by a nonzero parent_flag.
Returns nonzero if the name is ambiguous. */
-int
+bool
gfc_find_sym_tree (const char *name, gfc_namespace *ns, int parent_flag,
gfc_symtree **result)
{
Maybe change nonzero to true in the comment?
(...)
OK with all the above fixed.
Thanks.
[PATCH v2] Fortran: Narrow return types [PR78798]
From: Bernhard Reutner-Fischer
gcc/fortran/ChangeLog:
PR fortran/78798
* array.cc (compare_bounds): Use narrower return type.
(gfc_compare_array_spec): Likewise.
(is_constant_element): Likewise.
(gfc_constant_ac): Likewise.
* check.cc (dim_rank_check): Likewise.
* cpp.cc (gfc_cpp_init_options): Likewise.
(dump_macro): Likewise.
* cpp.h (gfc_cpp_handle_option): Likewise.
* dependency.cc (gfc_ref_needs_temporary_p): Likewise.
(gfc_check_argument_dependency): Likewise.
(gfc_check_fncall_dependency): Likewise.
(ref_same_as_full_array): Likewise.
* dependency.h (gfc_check_fncall_dependency): Likewise.
(gfc_dep_resolver): Likewise.
(gfc_are_equivalenced_arrays): Likewise.
* expr.cc (gfc_copy_ref): Likewise.
(gfc_kind_max): Likewise.
(numeric_type): Likewise.
* gfortran.h (gfc_at_end): Likewise.
(gfc_at_eof): Likewise.
(gfc_at_bol): Likewise.
(gfc_at_eol): Likewise.
(gfc_define_undef_line): Likewise.
(gfc_wide_is_printable): Likewise.
(gfc_wide_is_digit): Likewise.
(gfc_wide_fits_in_byte): Likewise.
(gfc_find_sym_tree): Likewise.
(gfc_generic_intrinsic): Likewise.
(gfc_specific_intrinsic): Likewise.
(gfc_intrinsic_actual_ok): Likewise.
(gfc_has_vector_index): Likewise.
(gfc_numeric_ts): Likewise.
(gfc_impure_variable): Likewise.
(gfc_pure): Likewise.
(gfc_implicit_pure): Likewise.
(gfc_elemental): Likewise.
(gfc_pure_function): Likewise.
(gfc_implicit_pure_function): Likewise.
(gfc_compare_array_spec): Likewise.
(gfc_constant_ac): Likewise.
(gfc_expanded_ac): Likewise.
(gfc_check_digit): Likewise.
* intrinsic.cc (gfc_find_subroutine): Likewise.
(gfc_generic_intrinsic): Likewise.
(gfc_specific_intrinsic): Likewise.
* io.cc (compare_to_allowed_values): Likewise. And remove
unneeded forward declaration.
* misc.cc (gfc_done_2): Likewise.
* parse.cc: Likewise.
* parse.h (gfc_check_do_variable): Likewise.
* primary.cc (gfc_check_digit): Likewise.
* resolve.cc (resolve_structure_cons): Likewise.
(pure_stmt_function): Likewise.
(gfc_pure_function): Likewise.
(impure_stmt_fcn): Likewise.
(resolve_forall_iterators): Likewise.
(resolve_data): Likewise.
(gfc_impure_variable): Likewise.
(gfc_pure): Likewise.
(gfc_unset_implicit_pure): Likewise.
* scanner.cc (wide_is_ascii): Likewise.
(gfc_wide_toupper): Likewise.
(gfc_open_included_file): Likewise.
(gfc_at_end): Likewise.
(gfc_at_eof): Likewise.
(gfc_at_bol): Likewise.
(skip_comment_line): Likewise.
(gfc_gobble_whitespace): Likewise.
* symbol.cc (gfc_find_symtree_in_proc): Likewise.
* trans-array.cc: Likewise.
* trans-decl.cc (gfc_set_decl_assembler_name): Likewise.
* trans-types.cc (gfc_get_element_type): Likewise.
(gfc_add_field_to_struct): Likewise.
* trans-types.h (gfc_copy_dt_decls_ifequal): Likewise.
(gfc_return_by_reference): Likewise.
(gfc_is_nodesc_array): Likewise.
* trans.h (gfc_can_put_var_on_stack): Likewise.
---
Bootstrapped without new warnings and regression tested on
x86_64-linux with no regressions, OK for trunk?
gcc/fortran/array.cc | 8 +++
gcc/fortran/check.cc | 2 +-
gcc/fortran/cpp.cc | 3 +--
gcc/fortran/cpp.h | 2 +-
gcc/fortran/dependency.cc | 8 +++
gcc/fortran/dependency.h | 6 ++---
gcc/fortran/expr.cc| 6 ++---
gcc/fortran/gfortran.h | 48 +++---
gcc/fortran/intrinsic.cc | 6 ++---
gcc/fortran/io.cc | 13 ++-
gcc/fortran/parse.cc | 2 +-
gcc/fortran/parse.h| 2 +-
gcc/fortran/primary.cc | 4 ++--
gcc/fortran/resolve.cc | 22 -
gcc/fortran/scanner.cc | 20
gcc/fortran/symbol.cc | 2 +-
gcc/fortran/trans-array.cc | 2 +-
gcc/fortran/trans-decl.cc | 2 +-
gcc/fortran/trans-types.cc | 6 ++---
gcc/fortran/trans-types.h | 6 ++---
gcc/fortran/trans.h| 2 +-
21 files changed, 81 insertions(+), 91 deletions(-)
diff --git a/gcc/fortran/array.cc b/gcc/fortran/array.cc
index be5eb8b6a0f..4b7c1e715bf 100644
--- a/gcc/fortran/array.cc
+++ b/gcc/fortran/array.cc
@@ -994,7 +994,7 @@ compare_bounds (gfc_expr *bound1, gfc_expr *bound2)
/* Compares two array specifications. They must be constant or deferred
shape. */
-int
+bool
gfc_compare_array_spec (gfc_array_spec *as1, gfc_array_spec *as2)
{
int i;
@@ -1039,7 +1039,7 @@ gfc_compare_array_spec (gfc_array_spec *as1,
gfc_array_spec *as2)
use the symbol as an implied-DO iterator.
