On Tue, Jan 5, 2016 at 2:35 PM, Jeff Law <l...@redhat.com> wrote:
> On 12/25/2015 09:08 AM, Ryan Burn wrote:
>>
>> This patch fixes issues with cilk_spawn where bad diagnostics are
>> emitted for expressions invoking conversion operators or constructor
>> calls (PR69024, PR68997).
>>
>> It also fixes an issue with a missing CLEANUP_POINT_EXPR that causes
>> an ICE when gimplifying code containing a temporary with a destructor
>> (PR69048)
>>
>> Bootstrapped and regression tested on x86_64-linux
>>
>> 2015-12-25 Ryan Burn<cont...@rnburn.com>
>>
>> PR cilkplus/69024, PR cilkplus/68997, PR cilkplus/PR69048
>> * cilk.c (cilk_detect_spawn_and_unwrap): Use recursive function
>> find_spawn to search for the CILK_SPAWN_STMT.
>> (cilk_ignorable_spawn_rhs_op): Also ignore COMPOUND_EXPR.
>> (find_spawn): New.
>> (is_conversion_operator_function_decl_p): New.
>> (is_conversion_operator_call_p): New.
>> (is_unary_constructor_aggr_init_p): New.
>> (is_conversion_operator_aggr_init_p): New.
>> (extract_free_variables): Don't extract the slot variable of
>> an AGGR_INIT_EXPR.
>> (create_cilk_wrapper_body): Add CLEANUP_POINT_EXPR to the
>> spawn expression.
>>
>> * gcc/testsuite/g++.dg/cilk-plus/CK/pr68997.cc : New test.
>>
>> * gcc/testsuite/g++.dg/cilk-plus/CK/pr69024.cc : New test.
>>
>> * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc : New test.
>>
>> * gcc/testsuite/g++.dg/cilk-plus/CK/pr68001.cc : Removed check
>> depending on bad diagnostics.
>
> Do you have a copyright assignment on file with the FSF? This seems large
> enough to require one.
>>
>>
>>
>> +static bool
>> +is_conversion_operator_function_decl_p (tree t) {
>> + if (TREE_CODE (t) != FUNCTION_DECL)
>> + return false;
>> +
>> + return DECL_NAME (t) && TREE_LANG_FLAG_4 (DECL_NAME (t));
>
> At the least the use of TREE_LANG_FLAG here ought to document what you're
> checking for. However, more likely the use is an indicator that you're
> doing something wrong.
>
> Similarly for the use of TREE_LANG_FLAG_0.
>
> Essentially those flags are allowed to have different meanings for each
> language. AFAICT that their meaning varies for C/C++, so using them in a
> shared file like this is definitely suspect.
>
I could additionally check that the language is c++ before checking
those flags, but I'm not sure I see a way around using them. For c++,
the cilk_spawn expression can be nested within implicit conversion
operators or constructors as in this case:
struct A { operator int() { return 0; } };
A f();
int x = cilk_spawn f();
So I have to check for the conversion operators or constructors when
traversing the expression to search for the spawn. cp-tree.h has
macros that could be used to avoid checking TREE_LANG_FLAG directly,
but I wasn't sure if shared code files are allowed to include c++
specific headers. The code could be split to have a different version
for c++, but that would be more significant changes and could lead to
some code redundancy.
Any suggestions?
>
> Each new function should have a block comment before it which briefly
> describes the function and return values. See other existing functions for
> examples.
>
>> + return VL_EXP_OPERAND_LENGTH (t) == 4
>> + && TREE_CODE (fn) == ADDR_EXPR
>> + && is_conversion_operator_function_decl_p (TREE_OPERAND (fn, 0));
>
> For multi-line expressions, please wrap them in a outer set of parenthesis.
> I think there was another instance earlier.
>
>
>
> Jeff
