On 05/20/2016 02:42 AM, Jakub Jelinek wrote: > On Tue, May 10, 2016 at 01:29:50PM -0700, Cesar Philippidis wrote: >> @@ -5796,12 +5796,14 @@ tree >> finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> { >> bitmap_head generic_head, firstprivate_head, lastprivate_head; >> - bitmap_head aligned_head, map_head, map_field_head; >> + bitmap_head aligned_head, map_head, map_field_head, oacc_reduction_head; >> tree c, t, *pc; >> tree safelen = NULL_TREE; >> bool branch_seen = false; >> bool copyprivate_seen = false; >> bool ordered_seen = false; >> + bool allow_fields = (ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP >> + || ort == C_ORT_ACC; >> > > Formatting. You want = already on the new line, or add () around the whole > rhs and align || below (ort &. > > Though, this looks wrong to me, does OpenACC all of sudden support > privatization of non-static data members in methods? > >> bitmap_obstack_initialize (NULL); >> bitmap_initialize (&generic_head, &bitmap_default_obstack); >> @@ -5810,6 +5812,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> bitmap_initialize (&aligned_head, &bitmap_default_obstack); >> bitmap_initialize (&map_head, &bitmap_default_obstack); >> bitmap_initialize (&map_field_head, &bitmap_default_obstack); >> + bitmap_initialize (&oacc_reduction_head, &bitmap_default_obstack); >> >> for (pc = &clauses, c = clauses; c ; c = *pc) >> { >> @@ -5829,8 +5832,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> t = OMP_CLAUSE_DECL (c); >> if (TREE_CODE (t) == TREE_LIST) >> { >> - if (handle_omp_array_sections (c, ((ort & C_ORT_OMP_DECLARE_SIMD) >> - == C_ORT_OMP))) >> + if (handle_omp_array_sections (c, allow_fields)) > > IMNSHO you don't want to change this, instead adjust C++ > handle_omp_array_sections* where it deals with array sections to just use > the is_omp variant; there are still other places where it deals with > non-static data members and I think you don't want to change those. That should be fixed now. It looks like I only needed to prevent handle_omp_array_sections_1 from calling omp_privatize_field for acc regions. So I modified handle_omp_array_sections* to take a c_omp_region_type argument instead of a bool is_omp to enable that. finish_omp_clauses should be ok because it already has a field_ok variable to guard that calls to omp_privatize_field. >> { >> remove = true; >> break; >> @@ -6040,6 +6042,17 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> omp_clause_code_name[OMP_CLAUSE_CODE (c)]); >> remove = true; >> } >> + else if (ort == C_ORT_ACC >> + && OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION) >> + { >> + if (bitmap_bit_p (&oacc_reduction_head, DECL_UID (t))) >> + { >> + error ("%qD appears more than once in reduction clauses", t); >> + remove = true; >> + } >> + else >> + bitmap_set_bit (&oacc_reduction_head, DECL_UID (t)); >> + } >> else if (bitmap_bit_p (&generic_head, DECL_UID (t)) >> || bitmap_bit_p (&firstprivate_head, DECL_UID (t)) >> || bitmap_bit_p (&lastprivate_head, DECL_UID (t))) >> @@ -6050,7 +6063,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE >> && bitmap_bit_p (&map_head, DECL_UID (t))) >> { >> - error ("%qD appears both in data and map clauses", t); >> + if (ort == C_ORT_ACC) >> + error ("%qD appears more than once in data clauses", t); >> + else >> + error ("%qD appears both in data and map clauses", t); >> remove = true; >> } >> else >> @@ -6076,7 +6092,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> omp_note_field_privatization (t, OMP_CLAUSE_DECL (c)); >> else >> t = OMP_CLAUSE_DECL (c); >> - if (t == current_class_ptr) >> + if (ort != C_ORT_ACC && t == current_class_ptr) >> { >> error ("% allowed in OpenMP only in %" >> " clauses"); >> @@ -6103,7 +6119,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> } >> else if (bitmap_bit_p (&map_head, DECL_UID (t))) >> { >> - error ("%qD appears both in data and map clauses", t); >> + if (ort == C_ORT_ACC) >> + error ("%qD appears more than once in data clauses", t); >> + else >> + error ("%qD appears both in data and map clauses", t); >> remove = true; >> } >> else >> @@ -6551,8 +6570,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> } >> if (TREE_CODE (t) == TREE_LIST) >> { >> - if (handle_omp_array_sections (c, ((ort & C_ORT_OMP_DECLARE_SIMD) >> - == C_ORT_OMP))) >> + if (handle_omp_array_sections (c, allow_fields)) >> remove = true; >> break; >> } >> @@ -6586,8 +6604,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> t = OMP_CLAUSE_DECL (c); >> if (TREE_CODE (t) == TREE_LIST) >> { >> - if (handle_omp_array_sections (c, ((ort & C_ORT_OMP_DECLARE_SIMD) >> - == C_ORT_OMP))) >> + if (handle_omp_array_sections (c, allow_fields)) >> remove = true; >> else >> { >> @@ -6616,6 +6633,9 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP) >> error ("%qD appears more than once in motion" >> " clauses", t); >> + else if (ort == C_ORT_ACC) >> + error ("%qD appears more than once in data" >> + " clauses", t); >> else >> error ("%qD appears more than once in map" >> " clauses", t); >> @@ -6627,6 +6647,27 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) >> bitmap_set_bit (&map_field_head, DECL_UID (t)); >> } >> } >> + else if (TREE_CODE (t) == TREE_LIST) >> + { >> + while (TREE_CODE (t = TREE_CHAIN (t)) == TREE_LIST) >> + ; > > This looks too ugly. Please avoid the assignment inside of TREE_CODE. > Better: > do > t = TREE_CHAIN (t); > while (TREE_CODE (t) == TREE_LIST); > or while loop. Also, I'm surprised you are adding a new whole if, if it is > something you hit only for C_ORT_ACC (why), then it should be also guarded > with ort == C_ORT_ACC. TREE_LIST should mean just type dependent array > section, on which IMHO nothing should be done. I removed this hunk because the error handling is still missing some cases, so I'll address it as with a follow up patch. Is this patch ok for trunk? Cesar