From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 83F803857C55 for ; Mon, 13 Feb 2023 12:11:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 83F803857C55 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676290267; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=7oub0HUnB0L73p4dmKIoxhiTvkNgkOkuUjiC2KzZ6yQ=; b=cdQMlaZ2OQy7gypill4GhjirdsboGp+LdQ5LVqwD4GCcPFEMoi3e60t0e3PUDcQ69m7gW8 fNp0d+n21Mw7c0bhguA3ALMeW/MdFy5c9dU1yRMxP5IyisrrfXTq2Dgn9AdJLjjCBbHjUA 5OFEFB+zSDqCtjj6Pdnj3lLtXp9mf28= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-385-Q3PkQG4TPq-Wt_8NF2FkiA-1; Mon, 13 Feb 2023 07:11:04 -0500 X-MC-Unique: Q3PkQG4TPq-Wt_8NF2FkiA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 996DB830F88; Mon, 13 Feb 2023 12:11:03 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 550C9492B03; Mon, 13 Feb 2023 12:11:02 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 31DCAxu93945395 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Mon, 13 Feb 2023 13:10:59 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 31DCAwK23945394; Mon, 13 Feb 2023 13:10:58 +0100 Date: Mon, 13 Feb 2023 13:10:58 +0100 From: Jakub Jelinek To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Subject: Re: [Ping^3] gomp: Various fixes for SVE types [PR101018] Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Feb 13, 2023 at 10:45:05AM +0000, Richard Sandiford wrote: > Ping^3 [https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606741.html] > > ---- > > Various parts of the omp code checked whether the size of a decl > was an INTEGER_CST in order to determine whether the decl was > variable-sized or not. If it was variable-sized, it was expected > to have a DECL_VALUE_EXPR replacement, as for VLAs. > > This patch uses poly_int_tree_p instead, so that variable-length > SVE vectors are treated like constant-length vectors. This means > that some structures become poly_int-sized, with some fields at > poly_int offsets, but we already have code to handle that. > > An alternative would have been to handle the data via indirection > instead. However, that's likely to be more complicated, and it > would contradict is_variable_sized, which already uses a check > for TREE_CONSTANT rather than INTEGER_CST. > > gimple_add_tmp_var should probably not add a safelen of 1 > for SVE vectors, but that's really a separate thing and might > be hard to test. Generally, OpenMP has lots of clauses on lots of different constructs and for SVE we need to decide what to do with them, and it would be better to cover them all in testsuite coverage, so something orders of magnitude larger than this patch provides and then there is OpenACC too. Can one add these non-constant poly_int sized types as members of aggregates? If yes, they need to be tested in addition to the plain vectors. >From data sharing clauses and others: 1) shared (implicit or explicit) - I'd say the non-constant poly_int sized stuff should be shared by passing around an address, rather than by copying it around by value which can be large (so say similar to aggregates rather than scalars), though feel free to argue otherwise 2) for the offloading stuff, I'd say we want to error or sorry at gimplification time, both for explicit or implicit map clause on target/target data/target {enter,exit} data and on explicit/implicit private and firstprivate clauses on target; while it could work fine with host fallback, generally the intention is to offload to a different device and neither PTX nor AMDGCN have anything similar to SVE and even for say potential ssh based offloading to aarch64 there is the possibility that the two devices don't agree on the vector sizes 3) private clause just creates another private variable of the same type, except for target I think it should generally work fine, but should be nevertheless test covered, say on parallel, task and some worksharing construct (e.g. for) and simd and also with allocate clause specifying allocators 4) firstprivate clause is similar to private, but we need to copy the initial value to it; e.g. in case of parallel, host teams or task such values are copied through compiler generated artificial struct that contains all the stuff needed to be propagated around (and except for task/taskloop then propagated back). For the SVE stuff I think it might be nice to put the non-constant sized stuff late in the artificial structure so that one can access the constant sized stuff using constant offsets 5) lastprivate similar to private with copying value back from one particular thread/lane (e.g. should be tested also on simd) 6) reduction/in_reduction/task_reduction - reductions are partly privatization clauses, for SVE only user defined reductions apply (declare reduction), but need to be tested in lots of constructs, parallel, for, simd, taskloop, task reductions and inscan reductions (explicit/implicit) 7) copyin - can the SVE vars be threadprivate (thread_local in C++ etc.)? If not, at least needs testing 8) linear clause hopefully will reject SVE stuff, but needs testing 9) affinity clause - we just parse/ignore, but still needs testing 10) aligned clause on simd - again, needs testing 11) allocate clause - as I said, for most of the data sharing clauses coverage needs to be without and with corresponding allocate clauses 12) depend clause - this one again needs testing, it just under the hood takes address of the passed in variable, so probably should just work out of the box 13) nontemporal clause on simd - probably works fine, but needs testing 14) uniform clause - this is on declare simd, one needs to test declare simd with the various cases (vector without clause, uniform, linear) 15) enter/link clauses - as I said in 2), these IMHO should be rejected 16) detach clause - the clause requires specific type, so I think should be ok 17) use_device_ptr/use_device_addr/is_device_ptr - offloading stuff, so like 2) 18) inclusive/exclusive clauses - see 6) above for inscan reductions 19) map/has_device_addr - see 2) 20) doacross - requires specific format with scalars, so just check it is properly rejected Rest of clauses don't have arguments or have integral/logical expression operands, so those should be ok. Now, if the above is too much work for GCC 13 (likely it is), I think it would be best to just make sure explicit/implicit shared clause works fine (i.e. 1) ), perhaps a few other easy ones like 12) and simply sorry on everything else for now and then incrementally handle it up later. Jakub