Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)

[Aldy Hernandez <aldyh@redhat.com>]

On 03/22/13 17:03, Iyer, Balaji V wrote:

> I have not fixed all the issues below (the big one that is left is the bultin function representation that Joseph Pointed out). I have fixed most of the other issues. All the things I have fixed are marked by "FIXED!"

Don't worry, I can work on the builtin function representation.

I am keeping a list of pending issues on the wiki 
(http://gcc.gnu.org/wiki/cilkplus-merge) with my name in parenthesis for 
items I am working on.  Particularly, I have added a sub-section for 
array notation items that have been pointed out in reviews but have not 
been completed.  I suggest you keep this list up to date as well, so we 
don't loose track of what has been pointed out.

> diff --git a/gcc/c-family/ChangeLog.cilkplus b/gcc/c-family/ChangeLog.cilkplus
> index 6591fd1..10db29b 100644
> --- a/gcc/c-family/ChangeLog.cilkplus
> +++ b/gcc/c-family/ChangeLog.cilkplus
> @@ -1,7 +1,11 @@
> +2013-03-22  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> +
> +	* c-pretty-print.c (pp_c_expression): Added ARRAY_NOTATION_REF case.
> +
>  2013-03-20  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>
>  	* c-common.c (c_define_builtins): When cilkplus is enabled, the

You can combine changelog entries into one entry.  This will make it 
easier when we merge into mainline.  So basically, add the 
c-pretty-print.c entry to the entry below it.

>> Non-static function declarations like this should not be inside a .c file.
>> If these functions are used outside this file, there should be an associated
>> header that declares them; include it in the .c file.  If only used inside the .c file
>> that defines them, make them static (and topologically sort static functions
>> inside a source file so that forward static declarations are only needed for cases
>> of recursion).
>>
>>> +/* Mark the FNDECL as cold, meaning that the function specified by FNDECL
>> is
>>> +   not run as is.  */
>>
>> The cold attribute means unlikely to be executed rather than "not run as is".
>> Maybe "not run as is" is what's relevant here, but I'm not clear why this attribute
>> would be useful for built-in functions at all - the documentation suggests it's
>> only relevant when a user defines a function themselves, and affects the code
>> generated for that function, so wouldn't be relevant at all for built-in functions.

I see you fixed this.  Since you are only fixing some of the items 
Joseph pointed out in this patch, please put "FIXED" below each item you 
did to aid in reviewing.

>>
>>> +void
>>> +array_notation_init_builtins (void)
>>
>> Other built-in functions use various .def files (builtins.def and the files it includes)
>> to avoid lots of repetitive code like this - can you integrate this with that
>> mechanism?  If you do so, then you should be able to avoid (or massively
>> simplify) functions like:
>>
>>> +/* Returns true if the function call specified in FUNC_NAME is
>>> +   __sec_implicit_index.  */
>>> +
>>> +bool
>>> +is_sec_implicit_index_fn (tree func_name)
>>
>> because code can use the BUILT_IN_* enum values to test whether a particular
>> function is in use - which is certainly cleaner than using strcmp against the
>> function name.

And here put "FIXED" if fixed, or "Aldy is going to work on this" or 
remove it altogether so it's not assumed that it was fixed by this patch 
since you're quoting it.

>>
>>> +/* Returns the first and only argument for FN, which should be a
>>> +   sec_implicit_index function.  FN's location in the source file is is
>>> +   indicated by LOCATION.  */
>>> +
>>> +int
>>> +extract_sec_implicit_index_arg (location_t location, tree fn) {
>>> +  tree fn_arg;
>>> +  HOST_WIDE_INT return_int = 0;
>>> +  if (!fn)
>>> +    return -1;
>>
>> Why the random check for a NULL argument?  If a NULL argument is valid
>> (meaning that it makes the code cleaner to allow such arguments rather than
>> making sure the function isn't called with them), this should be documented in
>> the comment above the function; otherwise, if such an argument isn't valid,
>> there is no need to check for it.
>
> I always tend to check for a null pointer before I access the fields in the structure. In this case it is unnecessary. In some cases (e.g. find_rank) there is a good chance a null pointer will be passed into the function and we need to check that and reject those.

I think what Joseph is suggesting is that if NULL is not valid, then the 
caller should check this.  But if NULL is valid, then it should be 
documented in the function comment at the top.

>>> +  if (TREE_CODE (fn) == CALL_EXPR)
>>> +    {
>>> +      fn_arg = CALL_EXPR_ARG (fn, 0);
>>> +      if (really_constant_p (fn_arg))
>>
>> I don't think really_constant_p is what's wanted;
>> <http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
>> Intel_Cilk_plus_lang_spec_2.htm>
>> says "The argument shall be an integer constant expression.", and such
>> expressions always appear in the C front end as INTEGER_CST.  So you can just
>> check for INTEGER_CST.
>
> What about C++? This function is shared by both C and C++.

Same thing for C++, but...

>
>>
>> Now a subtlety here is that the function argument will have been folded by this
>> point, meaning that cases that aren't integer constant expressions in C standard
>> terms will be wrongly allowed (both by the original code and by a version
>> checking against INTEGER_CST).  In such cases, the way to get things checked
>> correctly is to use a keyword rather than a built-in function - as with
>> __builtin_choose_expr or __builtin_shuffle, for example.  Since this operation
>> seems special in ways that built-in functions generally aren't, that seems
>> reasonable anyway.  So the code parsing this keyword would check that the
>> argument is an INTEGER_CST, of integer type (since INTEGER_CSTs can have
>> pointer type in GCC), like that for __builtin_choose_expr does.  It would then
>> quite likely create its own tree code for the operation, rather than using a
>> CALL_EXPR at all.  (It would need to manage converting to int, given how the
>> specification defines things in terms of a prototype for type int - so e.g. a
>> constant 1ULL << 32 would act like 0 if int is 32 bits, under the present
>> specification.)
>>
>> The specification doesn't seem very clear on to what extent the __sec_*
>> operations must act like functions (what happens if someone puts parentheses
>> around the __sec_* name, for example - that wouldn't work with the keyword
>> approach).  So the specification should be clarified there, but I think saying the
>> __sec_* operations are syntactically special, like keywords, is more appropriate
>> than requiring other uses to work.
>>
>>> +	return_int = (int) int_cst_value (fn_arg);
>>> +      else
>>> +	{
>>> +	  if (location == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (fn))
>>> +	    location = EXPR_LOCATION (fn);
>>> +	  error_at (location, "__sec_implicit_index parameter must be a "
>>> +		    "constant integer expression");
>>
>> The term is "integer constant expression" not "constant integer expression".
>
> FIXED!

...it looks like you're going to have to rework all this as a keyword.

For now, I suggest you check for INTEGER_CST as suggested, put a FIXME 
comment explaining that we need to rework this as a keyword, and add an 
entry to the wiki as a TODO item.  This way you can attack the rest of 
the easier/cosmetic changes Joseph is suggesting without getting bogged 
down by the keyword.

Also, can you follow up with the specification changes suggested?

> +   parts that should be executed only once that comes with array notation
> +   expressions.  */

You probably want a comma in there somewhere, or split this sentence 
somehow.

>
>>
>>> +/* Returns the rank of ARRAY through the *RANK.  The user can specify
>> whether
>>> +   (s)he wants to step into array_notation-specific builtin functions
>>> +   (specified by the IGNORE_BUILTIN_FN).
>>
>> The wording seems awkward; "Set *RANK to the rank of ARRAY, ignoring array-
>> notation-specific built-in functions if IGNORE_BUILTIN_FN." would be better.
>
> Yes, I agree with your wording. Thanks! and FIXED!

> +/* Sets *RANK of expression ARRAY, ignoring array notation specific built-in

^^^^^^^^^^^
Almost, "Set *RANK to the rank of ARRAY, ignoring...".

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

s/ans/and

s/stores in/stores them in/

>
>>
>>> +{
>>> +  size_t ii = 0;
>>> +  an_reduce_type dummy_type = REDUCE_UNKNOWN;
>>> +
>>> +  if (!node)
>>> +    return;
>>
>> Again, check for NULL argument without any mention in the comment that such
>> arguments are valid; remove unless there is a reason to make them valid.
>>
>>> +  else if (TREE_CODE (node) == TREE_LIST)
>>
>> What's NODE?  My first guess would have been an expression, but if a TREE_LIST
>> is possible that's clearly not the answer, so explain in the comment above the
>> function what NODE is.  (If a TREE_LIST is being used within expressions to store
>> something specific to array notation, don't do so - TREE_LIST is deprecated,
>> existing uses should be phased out in favour of more specific and less memory-
>> hungry datastructures and new uses should not be added.)
>
> FIXED! What is replacing tree-list? I have used tree-list in my later patches and in my Cilk Plus branch.

For list of things, probably vectors, etc.

> +/* Find all the scalar expressions in *TP and push it in DATA struct,

s/it/them

>>> +bool
>>> +is_builtin_array_notation_fn (tree func_name, an_reduce_type *type) {
>>> +  const char *function_name = NULL;
>>> +
>>> +  if (!func_name)
>>> +    return false;
>>
>> Another unexplained test for a NULL argument.  Again, explain what sort of
>> things FUNC_NAME may be.  (This is another function that should be using
>> BUILT_IN_* enum values rather than strcmp, if you rework how the built-in
>> functions are implemented.)

Still present in the current patch ??

>
>>
>>> +{
>>> +  if (!t || !contains_array_notation_expr (t))
>>> +    return t;
>>
>> Another check for NULL without a comment saying NULL is a valid argument.
>
> This function also can receive a null pointer.

Then, document that NULL is a valid argument please.

> +  /* Here we assign the array notation components to variable so that we can
> +     satisfy the exec once rule.  */
> +  for (ii = 0; ii < lhs_list_size; ii++)
> +    {
> +      tree array_node = (*lhs_list)[ii];
> +      tree array_begin = ARRAY_NOTATION_START (array_node);
> +      tree array_lngth = ARRAY_NOTATION_LENGTH (array_node);
> +      tree array_strde = ARRAY_NOTATION_STRIDE (array_node);
> +
> +      begin_var = build_decl (location, VAR_DECL, NULL_TREE,
> +			      integer_type_node);
> +      lngth_var = build_decl (location, VAR_DECL, NULL_TREE,
...
...

There's a big chunk of code here that AFAICT is not part of the review. 
  Did I miss something, or did this creep in somehow?

> +
> +  loop = push_stmt_list ();

Similarly here.  What's this bit part of?

> @@ -2407,7 +2533,42 @@ fix_array_notation_call_expr (tree arg)
>      count_down[ii] = XNEWVEC (bool, rank);
>
>    array_var = XNEWVEC (tree, rank);
> +
> +  loop = push_stmt_list ();
> +  for (ii = 0; ii < list_size; ii++)
> +    {
> +      tree array_node = (*array_list)[ii];
> +      if (array_node && TREE_CODE (array_node) == ARRAY_NOTATION_REF)
> +	{
> +	  tree array_begin = ARRAY_NOTATION_START (array_node);
> +	  tree array_lngth = ARRAY_NOTATION_LENGTH (array_node);

Also here.  Did I miss something?

> +* Cilk Plus Builtins::  Built-in functions that are part of Cilk Plus language
> +                        extension.

Should be "Built-in functions for the Cilk Plus language extension."

> diff --git a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> index 272ef41..82008c0 100644
> --- a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> +++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> @@ -3,16 +3,15 @@ typedef int (*foo)(int);
>  int main(int argc, char **argv)
>  {
>    int array[10], array2[10][10];
> -  // int array[10], array2[10], value, ii = 0;

Do not add commented out code.

> diff --git a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c
> index 5fb3680..fd128b1 100644
> --- a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c
> +++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c
> @@ -26,7 +26,7 @@ int main(int argc, char **argv)
>        array[ii] = 10;
>        array2[ii] = 5000000;
>      }
> -  array2[0:10:2] = array[0:10:2];
> +  array2[0:5:2] = array[0:5:2];

Is this change part of this review cycle, or is this something else?  If 
the latter, then submit it as a separate patch.

> diff --git a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> index 272ef41..82008c0 100644
> --- a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> +++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c
> @@ -3,16 +3,15 @@ typedef int (*foo)(int);
>  int main(int argc, char **argv)
>  {
>    int array[10], array2[10][10];
> -  // int array[10], array2[10], value, ii = 0;

Do not add commented out code.

Thanks for following up on this.

Could you tackle the changes I have suggested and repost the patch?

Thanks.