[OpenMP,C++] Allow classes with static members to be mappable

Hi Jakub,
Now in OpenMP 5.x, static members are supposed to be not a barrier for a class
to be target-mapped.

There is the related issue of actually providing access to static const/constexpr
members on the GPU (probably a case of https://github.com/OpenMP/spec/issues/2158)
but that is for later.

This patch basically just removes the check for static members inside
cp_omp_mappable_type_1, and adjusts a testcase. Not sure if more tests are needed.
Tested on trunk without regressions, okay when stage1 reopens?

Thanks,
Chung-Lin

2022-03-09  Chung-Lin Tang  <cltang@codesourcery.com>

gcc/cp/ChangeLog:

	* decl2.cc (cp_omp_mappable_type_1): Remove requirement that all
	members must be non-static; remove check for static fields.

gcc/testsuite/ChangeLog:

	* g++.dg/gomp/unmappable-1.C: Adjust testcase.

On Wed, Mar 09, 2022 at 07:04:24PM +0800, Chung-Lin Tang wrote:
> Now in OpenMP 5.x, static members are supposed to be not a barrier for a class
> to be target-mapped.
> 
> There is the related issue of actually providing access to static const/constexpr
> members on the GPU (probably a case of https://github.com/OpenMP/spec/issues/2158)
> but that is for later.
> 
> This patch basically just removes the check for static members inside
> cp_omp_mappable_type_1, and adjusts a testcase. Not sure if more tests are needed.
> Tested on trunk without regressions, okay when stage1 reopens?
> 
> Thanks,
> Chung-Lin
> 
> 2022-03-09  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> gcc/cp/ChangeLog:
> 
> 	* decl2.cc (cp_omp_mappable_type_1): Remove requirement that all
> 	members must be non-static; remove check for static fields.

I don't see anything useful left in cp_omp_mappable_type{,_1}.
In particular, starting with OpenMP 5.0, for both C and C++ we just say
that a mappable type is a complete type.  True, for C++ there is also the
"All member functions accessed in any target region must appear in a
declare target directive."
and similarly for Fortran, but that isn't something we really can check when
we ask whether something is a mappable type, that isn't a property of a
type, but a property of the target region.
In OpenMP 4.5 the special C++ mappable_type langhooks was useful, both for
the non-static data members and for virtual methods.

So, I think instead of your patch, we should just throw away
cp_omp_mappable_type{,_1}, and as C++ was the only one that had a special
langhook, I think we should kill the langhook altogether too, move
lhd_omp_mappable_type from langhooks.cc to omp-general.cc or so
as omp_mappable_type and use that instead of the langhooks or
cp_omp_mappable_type.
Now, the C++ FE has also cp_omp_emit_unmappable_type_notes while other FEs
don't, either we can just say that type doesn't have mappable type
like the C FE does, or perhaps just can emit a note that it isn't a mappable
type because it is incomplete (but then it would be nice to do the same
thing in the C FE too).

	Jakub

Hi all,

On 05.05.22 11:12, Jakub Jelinek via Gcc-patches wrote:
→ https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594082.html
> On Wed, Mar 09, 2022 at 07:04:24PM +0800, Chung-Lin Tang wrote:
→ https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591449.html
>> Now in OpenMP 5.x, static members are supposed to be not a barrier for a class
>> to be target-mapped.
>>
>> There is the related issue of actually providing access to static const/constexpr
>> members on the GPU (probably a case of https://github.com/OpenMP/spec/issues/2158)
>> but that is for later.
>>
>> This patch basically just removes the check for static members inside
>> cp_omp_mappable_type_1, and adjusts a testcase. Not sure if more tests are needed.
>> Tested on trunk without regressions, okay when stage1 reopens?
> I don't see anything useful left in cp_omp_mappable_type{,_1}.
> In particular, starting with OpenMP 5.0, for both C and C++ we just say
> that a mappable type is a complete type.  True, for C++ there is also the
> "All member functions accessed in any target region must appear in a
> declare target directive."
> and similarly for Fortran, but that isn't something we really can check [...]
I have now removed the hook as suggested.
> Now, the C++ FE has also cp_omp_emit_unmappable_type_notes while other FEs
> don't, either we can just say that type doesn't have mappable type
> like the C FE does, or perhaps just can emit a note that it isn't a mappable
> type because it is incomplete (but then it would be nice to do the same
> thing in the C FE too).

I looked into this – but I think only C++ has TYPE_MAIN_DECL; without being
able to get the DECL_SOURCE_LOCATION, adding an "inform" does not really help.

Instead of the omp-specific emit function, I now use cxx_incomplete_type_inform,
which avoids printing a pointless "note:"/inform if no source location is
available and uses common wording. (Visible with the added example, which
printed before for some errors the omp-specific inform and for others the
cp-generic inform.)

(Actually, the testcase shows some overdiagnostic - for the single
incomplete-type issue, there are 3 error message (two identical) and with
C++ trice the same inform/"note:". This is unchanged with this patch.)

The code could be simplified by removing the COMPLETE_TYPE_P condition before
cxx_incomplete_type_inform in cp/{decl,semantics}.cc, given that in
omp_mappable_type(), the only check besides error_node is !COMPLETE_TYPE_P.

The patch passes bootstrapping and regtesting on x86-64-gnu-linux.
OK?

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

On Wed, Jul 27, 2022 at 01:45:30PM +0200, Tobias Burnus wrote:
> OpenMP/C++: Allow classes with static members to be mappable
> 
> As this is the last lang-specific user of the omp_mappable_type hook,
> the hook is removed, keeping only a generic omp_mappable_type for
> incomplete types (or error_node).
> 
> gcc/c/ChangeLog:
> 
> 	* c-decl.cc (c_decl_attributes, finish_decl): Call omp_mappable_type
> 	instead of removed langhook.
> 	* c-typeck.cc (c_finish_omp_clauses): Likewise.
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-objcp-common.h (LANG_HOOKS_OMP_MAPPABLE_TYPE): Remove.
> 	* cp-tree.h (cp_omp_mappable_type, cp_omp_emit_unmappable_type_notes):
> 	Remove.
> 	* decl2.cc (cp_omp_mappable_type_1, cp_omp_mappable_type,
> 	cp_omp_emit_unmappable_type_notes): Remove.
> 	(cplus_decl_attributes): Call omp_mappable_type instead of
> 	removed langhook.
> 	* decl.cc (cp_finish_decl): Likewise; call cxx_incomplete_type_inform
> 	in lieu of cp_omp_emit_unmappable_type_notes.
> 	* semantics.cc (finish_omp_clauses): Likewise.
> 
> gcc/ChangeLog:
> 
> 	* gimplify.cc (omp_notice_variable): Call omp_mappable_type
>         instead of removed langhook.
> 	* omp-general.h (omp_mappable_type): New prototype.
> 	* omp-general.cc (omp_mappable_type):  New; moved from ...
> 	* langhooks.cc (lhd_omp_mappable_type): ... here.
> 	* langhooks-def.h (lhd_omp_mappable_type,
> 	LANG_HOOKS_OMP_MAPPABLE_TYPE): Remove.
> 	(LANG_HOOKS_FOR_TYPES_INITIALIZER): Remote the latter.
> 	* langhooks.h (struct lang_hooks_for_types): Remove
> 	omp_mappable_type.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/gomp/unmappable-1.C: Remove dg-error; remove dg-note no
> 	longer shown as TYPE_MAIN_DECL is NULL.
> 	* c-c++-common/gomp/map-incomplete-type.c: New test.
> 
> Co-authored-by: Chung-Lin Tang <cltang@codesourcery.com>

Ok, thanks.

	Jakub