This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n)
On Thu, 4 Oct 2012, Iyer, Balaji V wrote:
> >>>>Here is a link to the latest spec. This should clear several of the
> >>>>questions you are seeking.
> >>>>(http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
> >>>>Intel_Cilk_plus_lang_spec_2.htm#array)
This specification is much improved, especially as regards specifying the
types of section expressions. I'm not convinced that "the type of the
array being subscripted shall have a declared size" is properly defined in
standard terms, but I can guess reasonable semantics - that if the
array-to-pointer decay were considered not to occur in such a context,
then the expressions for the array being subscripted shall have array
type, not pointer type, and the array type shall not be one with
unspecified size (array[]), although it may be a VLA. For example, given
"int a[10];", it would be valid to say a[:] or (a)[:] but not (+a)[:]. I
don't, however, see any testcases at all in this patch for that particular
requirements - not even for the completely clear-cut cases, such as giving
an error for "extern int a[]; a[:];" or "int *a; a[:];".
Say expr1 through expr9 are expressions with side effects, and you have:
expr1[expr2:expr3:expr4] = expr5[expr6:expr7:expr8] + expr9;
The spec says "However, in such a statement, a sub-expression with rank
zero is evaluated only once." - that is, each of the nine expressions is
evaluated once. I don't see any calls to save_expr to ensure these
semantics, or any testcases that verify that they are adhered to.
(Are multidimensional section expressions valid when what you have is
pointers to pointers, e.g. "int ***p; p[0:10][0:10][0:10];"? I don't see
anything to rule them out, so I assume they are valid, but don't see
testcases for them either.)
Looking at the patch itself:
In find_rank you have error ("Rank Mismatch!"); - this is not a properly
formatted error message according to the GNU Coding standards (which
typically would not have any uppercase). I'd also suggest that when you
find a rank, you store (through a location_t * pointer) the location of
the first expression found with that rank, so if you then find a
mismatching rank you can use error_at to point to that rank and then
inform to point to the previous rank it didn't match.
I'm not convinced that your logic, falling back to examining each operand
for a generic expression, is correct to find the ranks of all kinds of
expressions. For example, there are rules:
* "The rank of a simple subscript expression (postfix-expression [
expression ]) is the sum of the ranks of its operand expressions. The rank
of the subscript operand shall not be greater than one." - how do you
ensure this rule? Where do you test for errors if the subscript has too
high a rank (both in the front-end code, and in the testsuite), and test
(in the testsuite) for cases where the subscript has rank 1?
* "The rank of a comma expression is the rank of its second operand." - I
don't see anything special to handle that. Are there testcases for rank
of comma expressions? Apart from testing rank, you may need to test how
they are evaluated (that each part, with independent rank, gets fully
evaluted in turn) - I don't see anything obvious in the code to handle
them appropriately.
In general, I'd say you should have tests in the testsuite for each
syntactic type of expression supported by GCC, both standard and GNU
extensions, testing how it interacts with section expressions - both valid
cases, and cases that are invalid because of rank mismatches. As another
example, you don't have tests of conditional expressions.
Where do you test (both in code, and testcases to verify errors) that "The
rank of each expression in a section triplet shall be zero."? What about
"The rank of the postfix expression identifying the function to call shall
be zero."? "A full expression shall have rank zero, unless it appears in
an expression statement or as the controlling expression of an if
statement."? (This means, I suppose, that uses such as initializers or
sizes in array declarators must be rejected.) I'd advise going through
each sentence in the relevant part of the spec that says something is
invalid and making sure you diagnose it and have a test of this.
Where in the patch you use int for the size of something (e.g. a list) on
the host, please use size_t.
In extract_array_notation_exprs you appear to be reallocating every time
something is added to a list (with XRESIZEVEC). It would probably be more
efficient to use the vec.h infrastructure for an automatically resizing
vector on which you push things.
In c_parser_array_notation you appear to be converting indices to
integer_type_node in some cases, not converting at all in other cases.
But the spec says "The expressions in a triplet are converted to
ptrdiff_t.", so you need to convert to target ptrdiff_t, not target int.
And there's a requirement that "Each of the expressions in a section
triplet shall have integer type.". So you need to check that, and give an
error if it doesn't have integer type, before converting - and of course
add testcases for each of the possible positions for an expression having
one that doesn't have integer type.
In c-typeck.c you disable some errors and warnings for expressions
containing array notations. I don't know where the later point is at
which you check for such errors - but in any case, you need testcases for
these diagnostics on those cases to show that they aren't being lost.
In invoke.texi you have:
+@opindex flag_enable_cilkplus
But @opindex is for the user-visible options, not for internal variables.
That is,
@opindex fcilkplus
would be appropriate.
In passes.texi you refer to "the Cilk runtime library (located in
libcilkrts directory)". But no such directory is added by this patch.
Only add references to it in documentation with the patch that adds the
directory.
--
Joseph S. Myers
joseph@codesourcery.com