This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PING^2]RE: [PING] RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n)


Hello Everyone,	
	Did anyone get a chance to review this patch?

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Iyer, Balaji V
> Sent: Tuesday, December 11, 2012 12:50 PM
> To: 'Joseph Myers'
> Cc: 'gcc-patches@gcc.gnu.org'
> Subject: [PING] RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n)
> 
> Hello,
> 	Did you get a chance to look at this patch? I submitted this ~1 month
> ago, so thought I would inquire its status.
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> > -----Original Message-----
> > From: Iyer, Balaji V
> > Sent: Monday, November 05, 2012 4:43 PM
> > To: Joseph Myers
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n)
> >
> > Hello Joseph,
> > 	Here is the fixed patch with all your changes and the ChangeLog
> > entries below.
> >
> > gcc/ChangeLog
> > 2012-11-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >
> >         * Makefile.in (C_COMMON_OBJS): Added c-family/array-notation-
> > common.o.
> >         * doc/passes.texi (Cilk Plus Transformation): Documented array
> >         notation and overall transformations for Cilk Plus.
> >         * doc/invoke.texi (C Dialect Options): Documented -fcilkplus flag.
> >         * doc/generic.texi (Storage References): Documented
> > ARRAY_NOTATION_REF
> >         tree addition.
> >
> > gcc/c-family/ChangeLog
> > 2012-11-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >
> >         * c-common.h (build_array_notation_expr): New function declaration.
> >         (ARRAY_NOTATION_ARRAY): Added new #define.
> >         (ARRAY_NOTATION_CHECK): Likewise.
> >         (ARRAY_NOTATION_START): Likewise.
> >         (ARRAY_NOTATION_LENGTH): Likewise.
> >         (ARRAY_NOTATION_STRIDE): Likewise.
> >         (ARRAY_NOTATION_TYPE): Likewise.
> >         (enum array_notation_reduce_type): Added new enumerator.
> >         * c-common.def: Added new tree ARRAY_NOTATION_REF.
> >         * c-common.c (c_define_builtins): Added a call to initialize array
> >         notation builtin functions.
> >         (c_common_init_ts): Set ARRAY_NOTATION_REF as typed.
> >         * c-pretty-print.c (pp_c_postfix_expression): Added
> ARRAY_NOTATION_REF
> >         case.
> >         * c.opt (-fcilkplus): Define new command line switch.
> >         * array-notation-common.c: New file.
> >
> > gcc/c/ChangeLog
> > 2012-11-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >
> >         * c-typeck.c (build_array_ref): Added a check to see if array's index
> >         is greater than one.  If true, then emit an error.
> >         (build_function_call_vec): Exclude error reporting & checking for
> >         builtin array-notation functions.
> >         (convert_arguments): Likewise.
> >         (c_finish_return): Added a check for array notations as a return
> >         expression.  If true, then emit an error.
> >         (c_finish_loop): Added a check for array notations in a loop condition.
> >         If true then emit an error.
> >         (lvalue_p): Added a ARRAY_NOTATION_REF case.
> >         * Make-lang.in (C_AND_OBJC_OBJS): Added c-array-notation.o.
> >         * c-parser.c (c_parser_compound_statement): Check if array notation
> code
> >         is used in tree, if so, then transform them into appropriate C code.
> >         (c_parser_expr_no_commas): Check if array notation is used in LHS or
> >         RHS, if so, then build array notation expression instead of regular
> >         modify.
> >         (c_parser_postfix_expression_after_primary): Added a check for colon(s)
> >         after square braces, if so then handle it like an array notation.  Also,
> >         break up array notations in unary op if found.
> >         (c_parser_direct_declarator_inner): Added a check for array notation.
> >         (c_parser_compound_statement): Added a check for array notation in a
> >         stmt.  If one is present, then expand array notation expr.
> >         (c_parser_if_statement): Likewise.
> >         (c_parser_switch_statement): Added a check for array notations in a
> >         switch statement's condition.  If true, then output an error.
> >         (c_parser_while_statement): Same as switch statement, but for a while.
> >         (c_parser_do_statement): Same as switch statement, but for a do-while.
> >         (c_parser_for_statement): Same as switch statement, but for a for-loop.
> >         (c_parser_unary_expression): Check if array notation is used in a
> >         pre-increment or pre-decrement expression.  If true, then expand them.
> >         (c_parser_array_notation): New function.
> >         * c-array-notation.c: New file.
> >
> > gcc/testsuite/ChangeLog
> > 2012-11-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >
> >         * gcc.dg/cilk-plus/array_notation/execute/execute.exp: New script.
> >         * gcc.dg/cilk-plus/array_notation/compile/compile.exp: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/errors.exp: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/sec_implicit_ex.c: New test.
> >         * gcc.dg/cilk-plus/array_notation/execute/comma_exp.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/conditional.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/exec-once.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/if_test.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/n-ptr_test.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/gather_scatter.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/builtin_func_double2.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/builtin_func_double.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/builtin_fn_custom.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/builtin_fn_mutating.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/array_test_ND.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/array_test2.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/execute/array_test1.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/sec_implicit_ex.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/gather_scatter.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/builtin_func_double2.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/array_test_ND.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/if_test.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/builtin_func_double.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/compile/array_test1.c: Likewise
> >         * gcc.dg/cilk-plus/array_notation/compile/array_test2.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/sec_implicit.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/sec_implicit2.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/rank_mismatch.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/parse_error.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/parse_error2.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/parse_error3.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/parse_error4.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/sec_reduce_max_min_ind.c:
> >         Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/decl-ptr-colon.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/fn_triplet_values.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/gather-scatter.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/misc.c: Likewise.
> >         * gcc.dg/cilk-plus/array_notation/errors/vla.c: Likewise.
> >
> > Thanks,
> >
> > Balaji V. Iyer.
> >
> >
> > > -----Original Message-----
> > > From: Joseph Myers [mailto:joseph@codesourcery.com]
> > > Sent: Friday, October 19, 2012 5:38 PM
> > > To: Iyer, Balaji V
> > > Cc: Richard Guenther; gcc-patches@gcc.gnu.org
> > > Subject: 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/402
> > > > >>>>97
> > > > >>>>-
> > > > >>>>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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]