Discussion:
[Patch, fortran] PR40196 - [F03] [F08] Type parameter inquiry (str%len, a%kind) and Complex parts (z%re, z%im)
Paul Richard Thomas
2018-10-27 19:03:47 UTC
Permalink
I was triggered to do this by one of the comments in response to Anton
Shterenlikht's standards survey. The comment was sufficiently
inconsiderate that my first thought was not to respond. However,
curiosity got the better of me... so said the dead cat!

There is a lot of this patch but it is (more or less) straight
forward. The tricky parts were to get the logic right in
gfc_match_varspec and in expr.c. One more step on the way to real
F2002 and F2008 compliance!

Bootstraps and regtests on FC28/x86_64 - OK for trunk?

Paul

2018-10-27 Paul Thomas <***@gcc.gnu.org>

PR fortran/40196
* dependency.c (are_identical_variables): Return false if the
inquiry refs are not the same.
(gfc_ref_needs_temporary_p): Break on an inquiry ref.
* dump_parse_tree.c (show_ref): Show the inquiry ref type.
* expr.c (gfc_free_ref_list): Break on an inquiry ref.
(gfc_copy_ref): Copy the inquiry ref types.
(find_inquiry_ref): New function.
(simplify_const_ref, simplify_ref_chain): Call it. Add new arg
to simplify_ref_chain.
(gfc_simplify_expr): Use the new arg in call to
simplify_ref_chain.
(gfc_get_full_arrayspec_from_expr, gfc_is_coarray): Break on
inquiry ref.
(gfc_traverse_expr): Return true for inquiry ref.
* frontend-passes.c (gfc_expr_walker): Break on inquiry ref.
* gfortran.h : Add enums and union member in gfc_ref to
implement inquiry refs.
* intrinsic.c : Fix white nois.
* match.c (gfc_match_assignment): A constant lavlue is an
error.
* module.c : Add DECL_MIO_NAME for inquiry_type and the mstring
for inquiry_types.
(mio_ref): Handle inquiry refs.
* primary.c (is_inquiry_ref): New function.
(gfc_match_varspec): Handle inquiry refs calling new function.
(gfc_variable_attr): Detect inquiry ref for disambiguation
with components.
(caf_variable_attr): Treat inquiry and substring refs in the
same way.
* resolve.c (find_array_spec): ditto.
(gfc_resolve_substring_charlen): If there is neither a charlen
ref not an inquiry ref, return.
(resolve_ref): Handle inqiry refs as appropriate.
(resolve_allocate_expr): ENtities with an inquiry ref cannot be
allocated.
* simplify.c (simplify_bound, simplify_cobound): Punt on
inquiry refs.
* trans-array.c (get_array_ctor_var_strlen): Break on inquiry
ref.
*trans-expr.c (conv_inquiry): New function.
(gfc_conv_variable): Retain the last typespec to pass to
conv_inquiry on detecting an inquiry ref.


2018-10-27 Paul Thomas <***@gcc.gnu.org>

PR fortran/40196
* gfortran.dg/inquiry_part_ref_1.f08: New test.
Bernhard Reutner-Fischer
2018-10-28 09:16:14 UTC
Permalink
On Sat, 27 Oct 2018 20:03:47 +0100
Paul Richard Thomas <***@gmail.com> wrote:

A few nits.
+ /* Pull an inquiry result out of an expression. */
+
+ static bool
+ find_inquiry_ref (gfc_expr *p, gfc_expr **newp)
+ {
+ gfc_ref *ref;
+ gfc_ref *inquiry = NULL;
+ gfc_expr *tmp;
+
+ tmp = gfc_copy_expr (p);
+
+ if (tmp->ref && tmp->ref->type == REF_INQUIRY)
+ {
+ inquiry = tmp->ref;
+ tmp->ref = NULL;
+ }
+ else
+ {
+ for (ref = tmp->ref; ref; ref = ref->next)
+ if (ref->next && ref->next->type == REF_INQUIRY)
+ {
+ inquiry = ref->next;
+ ref->next = NULL;
+ }
+ }
+
+ if(!inquiry)
missing space before open parenthesis
*************** typedef struct gfc_ref
*** 1960,1965 ****
--- 1963,1970 ----
}
ss;
+ inquiry_type i;
inq would be easier to understand and unambiguous imho.
+ /* Used by gfc_match_varspec() to match an inquiry reference. */
+
+ static bool
+ is_inquiry_ref (const char *name, gfc_ref **ref)
+ {
+ inquiry_type type;
+
+ if (name == NULL)
+ return false;
+
+ if (ref) *ref = NULL;
+
+ switch (name[0])
+ {
+ if (strcmp (name, "re") == 0)
+ type = INQUIRY_RE;
+ else
+ return false;
+ break;
+
+ if (strcmp (name, "im") == 0)
+ type = INQUIRY_IM;
+ else
+ return false;
+ break;
+
+ if (strcmp (name, "kind") == 0)
+ type = INQUIRY_KIND;
+ else
+ return false;
+ break;
+
+ if (strcmp (name, "len") == 0)
+ type = INQUIRY_LEN;
+ else
+ return false;
+ break;
+
+ return false;
+ }
Is the switch really worth it? I'd have used a plain chain of strcmp,
fwiw.
! switch (tmp->u.i)
! {
! if (!gfc_notify_std (GFC_STD_F2008, "re or im
part_refs at %C")) ! return MATCH_ERROR;
I guess RE and IM should be capitalised?
*************** gfc_variable_attr (gfc_expr *expr, gfc_t
*** 2358,2363 ****
--- 2521,2527 ----
gfc_ref *ref;
gfc_symbol *sym;
gfc_component *comp;
+ bool has_inquiry_part;
if (expr->expr_type != EXPR_VARIABLE && expr->expr_type !=
EXPR_FUNCTION) gfc_internal_error ("gfc_variable_attr(): Expression
isn't a variable"); *************** gfc_variable_attr (gfc_expr
*expr, gfc_t *** 2387,2392 ****
--- 2551,2561 ----
if (ts != NULL && expr->ts.type == BT_UNKNOWN)
*ts = sym->ts;
+ has_inquiry_part = false;
+ for (ref = expr->ref; ref; ref = ref->next)
+ if (ref->type == REF_INQUIRY)
+ has_inquiry_part = true;
you could break here
+
for (ref = expr->ref; ref; ref = ref->next)
switch (ref->type)
{
Index: gcc/fortran/trans-expr.c
===================================================================
*** gcc/fortran/trans-expr.c (revision 265411)
--- gcc/fortran/trans-expr.c (working copy)
*************** conv_parent_component_references (gfc_se
*** 2510,2515 ****
--- 2510,2549 ----
conv_parent_component_references (se, &parent);
}
+
+ static void
+ conv_inquiry (gfc_se * se, gfc_ref * ref, gfc_expr *expr,
gfc_typespec *ts)
+ {
+ tree res = se->expr;
+
+ switch (ref->u.i)
+ {
+ res = fold_build1_loc (input_location, REALPART_EXPR,
+ TREE_TYPE (TREE_TYPE (res)), res);
+ break;
+
+ res = fold_build1_loc (input_location, IMAGPART_EXPR,
+ TREE_TYPE (TREE_TYPE (res)), res);
+ break;
+
+ res = build_int_cst (gfc_typenode_for_spec (&expr->ts),
+ ts->kind);
+ break;
+
+ res = fold_convert (gfc_typenode_for_spec (&expr->ts),
+ se->string_length);
+ break;
+
+ gcc_unreachable ();
+ }
+ se->expr = res;
Don't you have to gfc_free_expr (se->expr) or gfc_replace_expr() ?

cheers,
Paul Richard Thomas
2018-10-28 11:31:10 UTC
Permalink
Hi Bernhard,

Thanks for going through the patch:
....snip....
Post by Bernhard Reutner-Fischer
missing space before open parenthesis
Corrected.
....snip....
Post by Bernhard Reutner-Fischer
inq would be easier to understand and unambiguous imho.
Why? inquiry_type seems fine to me.

....snip....
Post by Bernhard Reutner-Fischer
Is the switch really worth it? I'd have used a plain chain of strcmp,
fwiw.
I have done it. However, I might revert in order to combine the switch
block where I set the typespec for the primary expression.

....snip....
Post by Bernhard Reutner-Fischer
I guess RE and IM should be capitalised?
Done
Post by Bernhard Reutner-Fischer
you could break here
+
for (ref = expr->ref; ref; ref = ref->next)
switch (ref->type)
{
Done
Post by Bernhard Reutner-Fischer
Index: gcc/fortran/trans-expr.c
....snip...
Post by Bernhard Reutner-Fischer
Don't you have to gfc_free_expr (se->expr) or gfc_replace_expr() ?
No these are tree expressions not gfc_expr. No cleanup is needed.

I haven't added testcases for errors. Does anybody think that this is necessary?

Cheers

Paul
Thomas Koenig
2018-10-28 12:38:32 UTC
Permalink
Hi Paul,
Post by Paul Richard Thomas
Post by Bernhard Reutner-Fischer
inq would be easier to understand and unambiguous imho.
Why? inquiry_type seems fine to me.
I think Bernhard means the name of the member, i.

I think it makes sense to leave as it is - gfc_ref is a
struct that occurs a lot in complicated expressions, and the other
members are one and two letters, too.
Post by Paul Richard Thomas
....snip....
Post by Bernhard Reutner-Fischer
Is the switch really worth it? I'd have used a plain chain of strcmp,
fwiw.
I have done it. However, I might revert in order to combine the switch
block where I set the typespec for the primary expression.
Whatever suits you best.
Post by Paul Richard Thomas
I haven't added testcases for errors. Does anybody think that this is necessary?
Might not be a bad idea to run through at least each new error message
again.

There is one illwfL test case which ICEs:

$ cat b.f90
program main
character(len=:), allocatable :: a
allocate(a,source="abc")
a%len = 2
print *,a
end
$ gfortran b.f90
gimplification failed:
(integer(kind=4)) .a <nop_expr 0x7f138ae67740
type <integer_type 0x7f138acd15e8 integer(kind=4) public SI
size <integer_cst 0x7f138acbcdb0 constant 32>
unit-size <integer_cst 0x7f138acbcdc8 constant 4>
align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd15e8 precision:32 min <integer_cst
0x7f138acbcd68 -2147483648> max <integer_cst 0x7f138acbcd80 2147483647>
pointer_to_this <pointer_type 0x7f138acd89d8>>

arg:0 <var_decl 0x7f138b980ab0 .a
type <integer_type 0x7f138acd1738 integer(kind=8) public DI
size <integer_cst 0x7f138acbcb70 constant 64>
unit-size <integer_cst 0x7f138acbcb88 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd1738 precision:64 min <integer_cst
0x7f138acbcdf8 -9223372036854775808> max <integer_cst 0x7f138acbce10
9223372036854775807>
pointer_to_this <pointer_type 0x7f138ad057e0>>
used DI b.f90:1:0 size <integer_cst 0x7f138acbcb70 64>
unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>
chain <var_decl 0x7f138b980b40 a type <pointer_type 0x7f138ae82540>
used unsigned DI b.f90:2:0 size <integer_cst 0x7f138acbcb70
64> unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>>>>
b.f90:4:0:

4 | a%len = 2
|
internal compiler error: gimplification failed
0xb45602 gimplify_expr(tree_node**, gimple**, gimple**, bool
(*)(tree_node*), int)
../../trunk/gcc/gimplify.c:12568

Regards

Thomas
Paul Richard Thomas
2018-10-29 17:51:52 UTC
Permalink
Hi Thomas,

Thanks for finding the assignment a%len = 2 that escapes the check for
lvalues. I am back home tomorrow night and will investigate why this
one evades the trap. I think that an error test is needed in
expr.c(gfc_check_assign).

Cheers

Paul
Post by Thomas Koenig
Hi Paul,
Post by Paul Richard Thomas
Post by Bernhard Reutner-Fischer
inq would be easier to understand and unambiguous imho.
Why? inquiry_type seems fine to me.
I think Bernhard means the name of the member, i.
I think it makes sense to leave as it is - gfc_ref is a
struct that occurs a lot in complicated expressions, and the other
members are one and two letters, too.
Post by Paul Richard Thomas
....snip....
Post by Bernhard Reutner-Fischer
Is the switch really worth it? I'd have used a plain chain of strcmp,
fwiw.
I have done it. However, I might revert in order to combine the switch
block where I set the typespec for the primary expression.
Whatever suits you best.
Post by Paul Richard Thomas
I haven't added testcases for errors. Does anybody think that this is necessary?
Might not be a bad idea to run through at least each new error message
again.
$ cat b.f90
program main
character(len=:), allocatable :: a
allocate(a,source="abc")
a%len = 2
print *,a
end
$ gfortran b.f90
(integer(kind=4)) .a <nop_expr 0x7f138ae67740
type <integer_type 0x7f138acd15e8 integer(kind=4) public SI
size <integer_cst 0x7f138acbcdb0 constant 32>
unit-size <integer_cst 0x7f138acbcdc8 constant 4>
align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd15e8 precision:32 min <integer_cst
0x7f138acbcd68 -2147483648> max <integer_cst 0x7f138acbcd80 2147483647>
pointer_to_this <pointer_type 0x7f138acd89d8>>
arg:0 <var_decl 0x7f138b980ab0 .a
type <integer_type 0x7f138acd1738 integer(kind=8) public DI
size <integer_cst 0x7f138acbcb70 constant 64>
unit-size <integer_cst 0x7f138acbcb88 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd1738 precision:64 min <integer_cst
0x7f138acbcdf8 -9223372036854775808> max <integer_cst 0x7f138acbce10
9223372036854775807>
pointer_to_this <pointer_type 0x7f138ad057e0>>
used DI b.f90:1:0 size <integer_cst 0x7f138acbcb70 64>
unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>
chain <var_decl 0x7f138b980b40 a type <pointer_type 0x7f138ae82540>
used unsigned DI b.f90:2:0 size <integer_cst 0x7f138acbcb70
64> unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>>>>
4 | a%len = 2
|
internal compiler error: gimplification failed
0xb45602 gimplify_expr(tree_node**, gimple**, gimple**, bool
(*)(tree_node*), int)
../../trunk/gcc/gimplify.c:12568
Regards
Thomas
--
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein
Paul Richard Thomas
2018-10-28 20:19:18 UTC
Permalink
Hi Thomas,

I tried failing cases of that kind; or assignment to len/kind part refs and
returned correct errors. Must check where I was going wrong.

Paul from a chilly Garching-bei-Muenchen
Post by Thomas Koenig
Hi Paul,
Post by Paul Richard Thomas
Post by Bernhard Reutner-Fischer
inq would be easier to understand and unambiguous imho.
Why? inquiry_type seems fine to me.
I think Bernhard means the name of the member, i.
I think it makes sense to leave as it is - gfc_ref is a
struct that occurs a lot in complicated expressions, and the other
members are one and two letters, too.
Post by Paul Richard Thomas
....snip....
Post by Bernhard Reutner-Fischer
Is the switch really worth it? I'd have used a plain chain of strcmp,
fwiw.
I have done it. However, I might revert in order to combine the switch
block where I set the typespec for the primary expression.
Whatever suits you best.
Post by Paul Richard Thomas
I haven't added testcases for errors. Does anybody think that this is
necessary?
Might not be a bad idea to run through at least each new error message
again.
$ cat b.f90
program main
character(len=:), allocatable :: a
allocate(a,source="abc")
a%len = 2
print *,a
end
$ gfortran b.f90
(integer(kind=4)) .a <nop_expr 0x7f138ae67740
type <integer_type 0x7f138acd15e8 integer(kind=4) public SI
size <integer_cst 0x7f138acbcdb0 constant 32>
unit-size <integer_cst 0x7f138acbcdc8 constant 4>
align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd15e8 precision:32 min <integer_cst
0x7f138acbcd68 -2147483648> max <integer_cst 0x7f138acbcd80 2147483647>
pointer_to_this <pointer_type 0x7f138acd89d8>>
arg:0 <var_decl 0x7f138b980ab0 .a
type <integer_type 0x7f138acd1738 integer(kind=8) public DI
size <integer_cst 0x7f138acbcb70 constant 64>
unit-size <integer_cst 0x7f138acbcb88 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7f138acd1738 precision:64 min <integer_cst
0x7f138acbcdf8 -9223372036854775808> max <integer_cst 0x7f138acbce10
9223372036854775807>
pointer_to_this <pointer_type 0x7f138ad057e0>>
used DI b.f90:1:0 size <integer_cst 0x7f138acbcb70 64>
unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>
chain <var_decl 0x7f138b980b40 a type <pointer_type
0x7f138ae82540>
used unsigned DI b.f90:2:0 size <integer_cst 0x7f138acbcb70
64> unit-size <integer_cst 0x7f138acbcb88 8>
align:64 warn_if_not_align:0 context <function_decl
0x7f138ae83200 MAIN__>>>>
4 | a%len = 2
|
internal compiler error: gimplification failed
0xb45602 gimplify_expr(tree_node**, gimple**, gimple**, bool
(*)(tree_node*), int)
../../trunk/gcc/gimplify.c:12568
Regards
Thomas
Loading...