Hi Richard,
Please see my comments embedded below.
>-----Original Message-----
>From: Richard Guenther [mailto:richard.guent...@gmail.com]
>Sent: Monday, September 24, 2012 5:53 AM
>To: Iyer, Balaji V
>Cc: gcc-patches@gcc.gnu.org; al...@redhat.com; r...@redhat.com;
>l...@redhat.com
>Subject: Re: [PATCH] Cilk Plus merging to trunk (2 of n)
>
>On Sun, Sep 23, 2012 at 2:01 AM, Iyer, Balaji V <balaji.v.i...@intel.com>
>wrote:
>> Hello Everyone,
>> Attached, please find a patch that will implement Cilk Plus Array
>> Notations for
>the C compiler. Array notations are indented to allow programmers to directly
>express parallelism in their programs. Array notations can be used to possibly
>see
>a more predictable performance improvement, hardware resource-utilization and
>vectorization. To enable the compiler recognize array notation syntax, we have
>added a flag "-fcilkplus." If this flag is not used, none of these changes are
>visible
>to the compiler user. For more information and examples about array notations
>please see Chapter 4 in the Cilk Plus Specification
>(http://software.intel.com/sites/default/files/m/6/3/1/cilk_plus_language_specif
>ication.pdf).
>>
>> Here are the ChangeLog entries for the changes I have made in this patch.
>
>Just a few comments on the middle-end side:
>
>+/* Array Notation expression.
>+ Operand 0 is the array; operand 1 is the starting array index
>+ Operand 2 contains the number of elements you need to access.
>+ Operand 3 is the stride.
>+ Operand 4 is the element size measured in units of alignments of
>+ element type. */
>+DEFTREECODE (ARRAY_NOTATION_REF, "array_notation_ref", tcc_reference,
>+5)
>
>I suppose that similar to ARRAY_RANGE_REF the type of the expression specifies
>the size of the result which is an array itself? ARRAY_NOTATION_REF is then
>ARRAY_RANGE_REF with stride possibly != 1. Thus please instead change
>ARRAY_RANGE_REF to contain an explicit stride.
IIRC ARRAY_RANGE_REF holds the start index and the end-index with no stride.
Whereas ARRAY_NOTATION_REF holds start_index, the number of elements you need
to access in the array and the stride. So, they both hold different
information. Converting one to another can cause some implementation and
performance complexities when we have assignments that have different stride
while accessing the same number of elements. Also, I did not see much (if any)
usage of Array_range_ref in C or C++. The only place that I saw were using it
were in Fortran and Ada. Adding the extra field for one thing can require
modifications those front-ends. This I felt was a bit unnecessary and
excessive. I am replacing the ARRAY_NOTATION_REF tree with the appropriate
array_ref and a loop. Thus, having a tree that holds array notation
information made things look straightforward.
>
>+ case ARRAY_NOTATION_REF:
>+ /* Nothing should happen here. We just return ALL_DONE. */
>+ ret = GS_ALL_DONE;
>+ break;
>+
>
>I don't believe that. You should not restrict GENERIC to put gimplfied
>operands
>into the operand slots. See how ARRAY_RANGE_REF is handled.
>The exception may be if ARRAY_NOTATION_REF never survives until gimplfication
>(and thus is not part of GENERIC but a frontend specific tree).
ARRAY_NOTATION_REF does not survive gimplifcation. It gets broken up toward the
end of parsing. In hindsight, I should have done something like this:
case ARRAY_NOTATION_REF:
gcc_unreachable ();
Thanks,
Balaji V. Iyer.
>
>Richard.
>
>> gcc/ChangeLog:
>> 2012-09-22 Balaji V. Iyer <balaji.v.i...@intel.com>
>> * tree.h (array_notation_reduce_type): Added new enumerator.
>> (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.
>> * gimplify.c (gimplify_expr): Added a ARRAY_NOTATION_REF case.
>> * tree.def: Added new tree ARRAY_NOTATION_REF.
>> * Makefile.in (OBJS): Added array-notation-common.o.
>> * doc/passes.texi (Cilk Plus Transformation): Documented array
>> notation
>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.
>> * array-notation-common.c: New file.
>>
>> gcc/c/ChangeLog
>> 2012-09-22 Balaji V. Iyer <balaji.v.i...@intel.com>
>> * c-typeck.c (convert_arguments): Added a check if tree contains
>> array notation expressions before throwing errors or doing anything.
>> * 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_array_notation): New function.
>> * c-array-notation.c: New file.
>>
>> gcc/c-family/ChangeLog
>> 2012-09-22 Balaji V. Iyer <balaji.v.i...@intel.com>
>> * c-common.h (build_array_notation_expr): New function declaration.
>> * c-common.c (c_define_builtins): Added a call to initialize array
>> notation builtin functions.
>> * c.opt (-fcilkplus): Define new command line switch.
>>
>> gcc/testsuite/ChangeLog
>> 2012-09-21 Balaji V. Iyer <balaji.v.i...@intel.com>
>> * gcc.dg/cilk-plus/array_notation/execute/execute.exp: New script.
>> * gcc.dg/cilk-plus/array_notation/compile/compile.exp: New script.
>> * gcc.dg/cilk-plus/array_notation/execute/sec_implicit_ex.c: New
>> test.
>> * gcc.dg/cilk-plus/array_notation/execute/if_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/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.
>>
>>
>> I have tested this on x86 and x86_64 and passes all the applicable regression
>test. Is this OK for trunk?
>>
>> Thanking You,
>>
>> Yours Sincerely,
>>
>> Balaji V. Iyer.
