Re: [cilkplus] misc cleanups for <#pragma simd> implementation

[Aldy Hernandez <aldyh at redhat dot com>]

On 04/08/13 08:59, Iyer, Balaji V wrote:
> Hi Aldy,
> 	Here are the things I  found with the patch. All my comments have "BVI:" in front of them.

BTW, it would be nice if you could use standard mailer quotation when 
responding (">", etc).

> -  return;
>
> BVI: I am OK with removing this return, but the reason why I put it there is because it gets easier for me to set the break point there.

This is not standard practice in GCC source code.  It will have to be 
removed if we ever merge.

>       case PRAGMA_CILK_GRAINSIZE:
> -      if (context == pragma_external)
> -	{
> -	  c_parser_error (parser,"pragma grainsize must be inside a function");
> -	  return false;
> -	}
> -      if (flag_enable_cilk)
> -	c_parser_cilk_grainsize (parser);
> -      else
> -	{
> -	  warning (0, "pragma grainsize ignored");
> -	  c_parser_skip_until_found (parser, CPP_PRAGMA_EOL, NULL);
> -	}
> +      if (!c_parser_pragma_simd_ok_p (parser, context))
> +	return false;
> +      cilkplus_local_simd_values.loc = loc;
> +      c_parser_cilk_grainsize (parser);
>
> BVI: This is incorrect. #pragma grainsize is part of cilk keywords. It has no relation to the pragma simd and it will work wthout vectorization support.

Fixed, let me know if the current implementation is correct.

> +// FIXME: We should really rewrite all this psv* business to use vectors.
> +/* Given an index into the pragma simd list (PSV_INDEX), find its
> +   entry and return it.  */
>
> BVI: I am in the process of doing so. I will send out that patch as soon as I get some free time.

Ok, I have left all the FIXMEs so we don't miss any of them.

>     for (ps_iter = psv_head; ps_iter->ptr_next != NULL;
>          ps_iter = ps_iter->ptr_next)
> -    {
> -      ;
> -    }
> +    ;
>
> BVI: Are you sure the compiler let you get away this this? It gave me a warning once (in stage2 I believe).

Sure, I've done it for years.

> BVI: I have fixed these scripts already: the correct notation that I have used is "cilkplus_<type>_<language>_<compile/execute/errors>.exp

Ok, I have removed them from my patch.
> -
> +	
> BVI: Why did you replace a space with a tab?

Whoops, removed the space (and the tab).

How about this?

Attachment: curr
Description: Text document