Yuao Ma wrote:
This patch introduces support for the NIL value within conditional
arguments, addressing several scenarios for how NIL should be handled.
Let's start with two testcases that fail:
(A) Passing a string argument
I found a test that fails with an ICE (segmentation fault)
in generic_simplify_COND_EXPR, called by gfc_conv_conditional_expr:
implicit none
interface
subroutine sub(x)
character(*), optional :: x
end
end interface
logical :: cc
call sub( (cc ? "132" : .nil.))
end
I have not checked, but I assume that it has a NULL_TREE
instead of passing '0' as length. I think that should be
gfc_index_zero_node but I have not checked.
(I think gfc_index_zero_node is of type signed with the
same size as 'size_t'/size_type_node - and I think that
is what gfortran uses for the hidden length (
* * *
(B) OPTIONAL + VALUE
In that case, a null-pointer cannot be used. GCC follows the IBM
compiler by passing a hidden argument to denote the present
status.
The patch does not handle this case, passing always .true.:
sub (1, D.4682 ? 1 : (void) 0, 1);
sub (0, D.4683 ? 1 : (void) 0, 1);
for
module m
implicit none
contains
subroutine sub(expected, x)
logical, value :: expected
integer, value, optional :: x
if (expected .neqv. present(x)) error stop
end
subroutine test
logical :: cc
cc = .true.
call sub(.true., (cc ? 1 : .nil.))
cc = .false.
call sub(.false., (cc ? 1 : .nil.))
end
end
use m
call test
end
* * *
[Added after writing the following.
Reading through your text, I see that you already mentioned
that some features will be handled in follow-up patches,
like C1542. The latter also mentions coarrays. Thus, I think
you can leave this one out (i.e. keep the current error and
don't add a 'sorry' here), fixing the issues later.]
(C) I now tried scalar coarrays – and those fail as well.
I got the idea because for them also hidden arguments are
passed on.
The simplest way is to compile with -fcoarray=single, which
mostly ignores all coarrays - and 'this_image()' is always
one.
If you compile with '-fcoarray=lib' actual library calls are
inserted. If you link with '-lcaf_single', that's a stub
library which permits only a single image.
If you want to try with real coarray support, you need the
http://www.opencoarrays.org/ library - and link it instead
of libcaf_single.so, but that shouldn't be needed here and
for the code gen, it is identical to the caf_single case.
(There is a subdirectory in testsuite/gfortran.dg/ that
handles coarray testing.)
module m
implicit none
integer :: my_coarray[*], my_coarray2[*]
contains
subroutine sub(expected, x)
logical, value :: expected
integer, optional :: x[*]
integer :: image
image = this_image()
if (expected .neqv. present(x)) error stop 1
if (present(x)) then
if (x[image] /= 1) error stop 2
end if
end
subroutine sub2(x)
integer :: x[*]
integer :: image
image = this_image()
if (x[image] /= 2) error stop 3
end
subroutine test
logical :: cc
cc = .true.
! Bogus error: Actual argument to ‘x’ at (1) must be a coarray
call sub2((cc ? my_coarray2 : my_coarray))
call sub(.true., (cc ? my_coarray : .nil.))
cc = .false.
call sub(.false., (cc ? my_coarray : .nil.))
end
end
use m
my_coarray = 1
my_coarray2 = 2
call test
end
* * *
NOTE: Besides the case of passing coarrays as above, one can also have
allocatable coarrays. It works mostly similar, but the token is handled
differently. Try:
subroutine sub(x)
integer, allocatable :: x[:]
and then pass an allocatable coarray to it.
* * *
Can you check the cases above? I think the first two should really
work.
* * *
Side remark: I was wondering about the diagnostic for:
subroutine sub(x)
character(5), optional :: x
and passing a too-short string.
Result: No message for:
call sub( (cc ? "132" : "12"))
but for
call sub( (cc ? "132" : "345"))
we get:
Warning: Character length of actual argument shorter
than of dummy argument ‘x’ (3/5) at (1)
Thus, this part seems to work FINE :-)
(It is possible to warn for the first case, but it is
rather special, hence, I don't expect a warning for it
by adding a specific test for it.)
* * *
We now support three distinct behaviors for NIL argument:
- accept it for actual arguments to OPTIONAL dummy arguments
- reject it for dummy arguments that are not OPTIONAL
- reject it when not used as actual argument.
It is also rejected when used as, e.g., '1 + (c ? var : .nil.)'
as it should.
* * *
Implementation Details
- The NIL node is represented by an expression of type
EXPR_CONDITIONAL where the condition field is nullptr.
- To distinguish between the valid and invalid uses mentioned above,
we rely on the global state actual_arg to determine if the expression
is being used as an actual argument.
... which permits the diagnostic mentioned above. (In case of doubt:
The patch only adds another use case for actual_arg, the global variable
is not actually new.)
This patch does not yet address diagnostics for intrinsic procedures.
Handling NIL arguments for intrinsics is complex because most
intrinsic functions already utilize their own custom argument-checking
functions. While one option would be to add a new loop to the
check_specific - traversing the argument list and verifying the
OPTIONAL status - I haven't yet committed to this approach, as I'm
unsure if it's the most optimal solution.
Note that the use of 'optional' in intrinsic functions is also
rather limited.
Most don't permit optional arguments - and some have an optional
argument but many, it means either no argument or a present argument
and not a dummy argument that might be absent.
Thus, while this is an omission, I think it has nearly zero effect
on real-world code.
* * *
Regarding the Fortran 2023 standard, C1538 - C1545 outline additional
requirements for conditional arguments involving NIL. I plan to
gradually expand support to meet these extra requirements over
subsequent patches.
Some are handled (as they are the same as existing tests), others
are argument specific.
On the other hand, some features are similar like:
implicit none
integer, pointer :: p
integer, target :: x
integer, target :: y
logical :: cond
cond = .true.
p => (cond ? x : y)
end
This fails with:
Error: Pointer assignment target is neither TARGET nor POINTER at (1)
We can argue about the wording – but an error is correct as this
example is INVALID - pointer assignment requires either
a designator or a function reference that returns a pointer.
Thus, when fixing the current code to handle the following (valid)
example, we still have to print an error for the code above.
subroutine s(x)
integer, pointer :: x
...
call s( (cond ? ptr1 : ptr2) ) ! valid
* * *
--- a/gcc/fortran/expr.cc
+++ b/gcc/fortran/expr.cc
@@ -136,6 +136,21 @@ gfc_get_conditional_expr (locus *where, gfc_expr
*condition,
return e;
}
+/* Check if the condtional expression has .nil. or not */
+
+bool
+gfc_is_conditional_nil (gfc_expr *e)
+{
+ if (!e)
+ gcc_unreachable ();
IMHO, that should either be 'gcc_assert (e != nullptr)'
or 'gcc_checking_assert (e != nullptr);'
That's internally (system.h)
#if ENABLE_ASSERT_CHECKING
#define gcc_assert(EXPR) \
((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
#elif (GCC_VERSION >= 4005)
#define gcc_assert(EXPR) \
((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0))
...
or
#if CHECKING_P
#define gcc_checking_assert(EXPR) gcc_assert (EXPR)
#else
/* N.B.: in release build EXPR is not evaluated. */
#define gcc_checking_assert(EXPR) ((void)(0 && (EXPR)))
* * *
+ if (e->expr_type != EXPR_CONDITIONAL)
+ return false;
+ if (e->value.conditional.condition == nullptr)
+ return true;
+ return gfc_is_conditional_nil (e->value.conditional.true_expr)
+ || gfc_is_conditional_nil (e->value.conditional.false_expr);
+}
Looking at the last two line and at (BTW: there is a typo in 'cond(i)tional')
/* Check if the condtional expression has .nil. or not */
…
gfc_is_conditional_nil (gfc_expr *e)
I am not really happy with the name – from the function name, I'd
expect that it checks that 'e' is .NIL.
In really it checks whether the expression is or contains .NIL.,
but it only checks the latter if 'e' is a conditional expression.
It is currently used to check whether an actual argument contains
a '.nil.' - to reject it if the dummy argument is not optional.
It does not handle, e.g., '1 + (cond ? ... : .nil.)' but this is
already properly rejected (as mentioned above).
The 'is itself .NIL.' check is in principle unintended, but as
the function is recursively called -
to handle '(c ? e1 : c2 ? e2 : .nil.)')
- the .NIL. check for 'e' itself makes algorithmically sense.
Back to the wording question. How about:
/* Check whether the conditional expression contains .NIL.
(or is .NIL. itself). */
…
gfc_conditional_expr_with_nil
I think that's not ideal either, but should a bit clearer.
* * *
@@ -1409,7 +1424,7 @@ simplify_conditional (gfc_expr *p, int type)
|| !gfc_simplify_expr (false_expr, type))
return false;
- if (!gfc_is_constant_expr (condition))
+ if (!condition || !gfc_is_constant_expr (condition))
return true;
I think that's clearer as:
if (!condition /* is .NIL. */
|| !gfc_is_constant_expr (condition))
return true;
because the internal representation of .NIL. is not obvious
and contrary to all other code dealing with .NIL. it is also
not obvious from the context.
* * *
+ if (a->expr->expr_type == EXPR_CONDITIONAL
+ && gfc_is_conditional_nil (a->expr) && !optional_dummy)
+ {
+ gfc_error (
+ "Dummy argument is not optional, .NIL. at %L shall not appear",
+ &a->expr->where);
That's a comma splice; I think it reads better as:
"As the dummy argument is not optional, .NIL. at %L shall not appear"
* * *
- if (e->expr_type != EXPR_VARIABLE)
+ if (e->expr_type != EXPR_VARIABLE && e->expr_type != EXPR_CONDITIONAL)
I wonder whether we should do here as well:
if (e->expr_type != EXPR_VARIABLE
&& e->expr_type != EXPR_CONDITIONAL /* not .NIL. */)
to make clear why only EXPR_CONDITIONAL has a special case.
* * *
@@ -12770,6 +12770,9 @@ gfc_walk_conditional_expr (gfc_ss *ss, gfc_expr *expr)
{
gfc_ss *head;
+ if (!expr->value.conditional.condition)
+ return gfc_ss_terminator;
For now … Obviously, it needs to be revisited when adding array support.
* * *
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -4375,6 +4375,12 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr *expr)
tree condition, true_val, false_val;
tree type;
+ if (!expr->value.conditional.condition)
+ {
+ se->expr = build_empty_stmt (input_location);
This requires more for character strings, VALUE+OPTIONAL etc.
as discussed above.
Can you add '/* Handle .NIL. */ above? It is not obvious
from the code that this is about .NIL. - and can also not
be gathered by looking a few lines above or below. Thus,
the comment will help.
* * *
+program conditional_nil
+ implicit none
+ integer :: a = 4
+ integer :: b = 6
+
+ call five((a < 5 ? a : .NIL.))
+ if (a /= 5) stop 1
+ call five((a == 5 ? .NIL. : a))
+ if (a /= 5) stop 2
This test is not ideal: 'five' sets 'a = 5' but you already have 5.
How about using, e.g.,
a = 42
call five((a == 42 ? .NIL. : a))
if (a /= 42) stop 2
here?
* * *
+ call five((a /= 5 ? .NIL. : b > 5 ? b : .NIL.))
+ if (b /= 5) stop 3
Same issue, exceptionally, here.
* * *
+++ b/gcc/testsuite/gfortran.dg/conditional_11.f90
[…]
+ i = (i > 0 ? 1 : .nil.) ! { dg-error "is only valid in conditional
arguments" }
+ i = (i > 0 ? .nil. : .nil.) ! { dg-error "Both operands of the conditional
argument at" }
+ call five((i < 5 ? i : i > 43 ? i : .nil.)) ! { dg-error "Dummy argument is not
optional, .NIL. at" }
The second line is wrong for two reasons: (A) at least one
expression must be not .NIL. and (B) .NIL. is only valid in
conditional arguments.
I think it is better to add a new subroutine - that takes
an optional argument - and then check for (B) only.
That's again just a minor test nit - as the code works
and effectively also this check.
* * *
Can you also add some test for an actual argument with
conditions that is not a conditional argument, e.g.,
call five ( 1 + (cond ? 1 : .NIL. ) )
In that case, .NIL. is invalid - and the code handles this;
however, a testcase is missing for it.
Otherwise, it looks good.
Thanks!
Tobias
