RE: [patch] cilkplus: Array notation for C patch

["Joseph S. Myers" <joseph at codesourcery dot com>]

Here's a review of the changes to the compiler proper in this patch.
I don't think much more will come up from reviews of the compiler
changes - but I still need to review the testsuite changes against the
language specification to make sure that everything is properly
covered in the testsuite (which might in turn show up further things
needing to be addressed in the compiler).

> +	  error_at (location, "__sec_implicit_index parameter must be a " 
> +		    "integer constant expression");

"an", not "a".

> diff --git a/gcc/c/ChangeLog.cilkplus b/gcc/c/ChangeLog.cilkplus

I believe the actual trunk commit, when this is ready to go in, should
simply add the ChangeLog entries for the committed changes to the top
of the existing ChangeLog files, rather than creating such a new
ChangeLog file.

> diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c

> +#include "gcc.h"

That header is for the compiler driver.  Including it in anything
built into cc1 is suspicious.

> +/* Given an FNDECL or an ADDR_EXPR, return the corresponding

I think you mean something like "Given FNDECL, a FUNCTION_DECL or an
ADDR_EXPR", rather than "Given an FNDECL or an ADDR_EXPR".

> +/* Set *RANK of expression ARRAY, ignoring array notation specific built-in 
> +   functions if IGNORE_BUILTIN_FN is true.  The ORIG_EXPR is printed out if an
> +   error occured in the rank calculation.  The functions returns false if it
> +   encounters an error in rank calculation.
> +
> +   For example, an array notation of A[:][:] or B[0:10][0:5:2] or C[5][:][1:0]
> +   all have a rank of 2.  */

This still doesn't seem to say anything about the semantics of the
value *RANK on entry to the function.  (I think it's something like
*RANK being either 0, or the rank of another subexpression that must
have the same rank as this one, but you need to say that.)

> +/* Extracts all array notations in NODE and stores them in ARRAY_LIST.  If 
> +   IGNORE_BUILTIN_FN is set, then array notations inside array notation
> +   specific built-in functions are ignored.  The NODE can be anything from a
> +   full function to a single variable.   */

"can be anything"?  That seems rather ad hoc.  I'd think there should
be defined classes of trees - probably expressions and things that can
appear in them, but not tcc_exceptional or tcc_type - that can appear
here, and that you should check (in an assertion) for EXPR_P or one of
the other cases allowed.

In particular, you allow TREE_LIST in this function.  How can
TREE_LISTs get here and can they readily be avoided?  It's generally a
bad idea (and rare) to have places where something with the static
type "tree" can be either a TREE_LIST or some other kind of tree.  I
note that in the function replace_array_notations, which is presumably
intended to match this one, you *don't* handle TREE_LIST.

These functions recurse down into operands of trees.  But what about
into types?  If a type contains an expression that needs to be
evaluated as part of evaluating VLA sizes, that gets stored specially
by grokdeclarator, and in the end that expression get put in a
statement somewhere to ensure that it does get evaluated.  But that's
for expressions with side effects involved in types.  Array notation
expressions may not necessarily have side effects.  And as I
understand it, even if an expression is extracted OK by
extract_array_notation_exprs because it appears somewhere that
function looks at, replace_array_notations will need to substitute it
everywhere - substituting a copy appearing directly in a statement /
expression, while missing a copy embedded in a type, won't suffice.
So maybe you need to recurse down into types in some way?  (Then I'm
not entirely sure when it's safe to modify an existing type and when
you'd need to build up a new, similar type with the expression
modified appropriately.)

Maybe an example would help.  I see nothing in the Cilk Plus
specification to rule out expressions of the form

a[:] = ((int (*)[b[:]][c[:]]) d[:])[1][2];

meaning that each element of the array d should be cast to a
pointer-to-VLA type, with the dimensions of the VLA coming from
corresponding elements of arrays b and c, and then element[1][2] of
that VLA extracted.  But the rules for determining rank don't really
seem to consider subexpressions that appear within types, so maybe
adjustments are needed there as well.  (Of course such type names can
appear within expressions in sizeof, or compound literals, or several
other cases in the syntax, not just in casts.)

It's possible that the above case does work despite types not being
adjusted, because the logic to multiply by array sizes when doing
pointer addition / array dereference may already have taken effect
while the expressions were constructed.  But leaving types unadjusted
still seems rather risky, and would seem likely to cause problems with
debug info (consider the case where a variable is actually being
declared with the type involving array notation, in a GNU C statement
expression, for example) if nothing else.

(I suppose changing the specification to disallow array notation
within types would be one way to avoid a lot of such issues.)

> +/* Top-level function to replace ARRAY_NOTATION_REF in a conditional statement
> +   in STMT.  */
> +
> +tree
> +fix_conditional_array_notations (tree stmt)

This comment doesn't seem to say anything abou the return value
semantics.

> +/* Returns true of EXPR (and its subtrees) contain ARRAY_NOTATION_EXPR node.  */

"true if", not "true of".

> +/* Walks through tree node T and find all the call-statments that do not return

Statements, not statments.

> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 2ae4622..f051ab5 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cgraph.h"
>  #include "plugin.h"
>  
> +
>  
>  /* Initialization routine for this file.  */
>  

Spurious diff hunk just adding whitespace.

> +  if (flag_enable_cilkplus && contains_array_notation_expr (cond))
> +    {
> +      error_at (start_locus, "array notation expression cannot be used in a "
> +		"loop%'s condition");
> +      return;
> +    }
> +  if (flag_enable_cilkplus && contains_array_notation_expr (incr) && 0)
> +    {
> +      error_at (start_locus, "array notation expression cannot be used in a "
> +		"loop's increment expression");
> +      return;
> +    }

Use %' in the second error here, as you did in the first.

> diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c

> +    case ARRAY_NOTATION_REF:

Since this is a C-family tree node, I'd think the handling in
c-pretty-print.c should suffice, without needing anything in
tree-pretty-print.c as well.

-- 
Joseph S. Myers
joseph@codesourcery.com