From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) by sourceware.org (Postfix) with ESMTPS id 44E4B3858435; Fri, 29 Oct 2021 17:15:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 44E4B3858435 Received: by mail-wr1-x435.google.com with SMTP id z14so17316430wrg.6; Fri, 29 Oct 2021 10:15:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=3JYeUTkGm2I2a1kELHEslwXcH3Kp/51MOEha2In5h5s=; b=yhkvh2vCbbW6VRmsfe6+5NOLJtIQ3jkwYRxOnfAN5vvh70KzM98SQXxa5KuFmUxCN1 W4Y7on0gCSvLbeMx+PB+8jNcpWKWjjOh5wdiVNH+VN5yqye1PqsC0JR1g/lvyGa5C6tQ 3LJ4T8YR3nf9AjJ+grY1BRiKYM3UMSwx7gXHxKskZtyk0oKwe0g+NpmdZsOBzjFpLnDS tFshuLOwQ2vMoPdt5E2k8ljDkqyBIw5j3/zQsxsbnqhsUwLFZdyB1+MWLapz3oqiBYN3 tx7WF0Iz08I8jqIUENcttt7GNPTNc4sp4TuY1tlsyneN4zQpzGN+xE1j8rV1GhkSWB5w ikpA== X-Gm-Message-State: AOAM531UGQFq6mRY1F9M5/2O7hBSQvclTvk3oyBQULJNUyZ/47+Gdz3Y JIADDVgfhx5+T5uCrd1alZ0= X-Google-Smtp-Source: ABdhPJyW+5oqrm3760orkBNB1Gmhl4K9oq+ZfdKUn0QMypAg+8ZTE9MGBHphqECT6Mr5UxLp0G0Wwg== X-Received: by 2002:adf:fc49:: with SMTP id e9mr15894070wrs.130.1635527717113; Fri, 29 Oct 2021 10:15:17 -0700 (PDT) Received: from nbbrfq (dynamic-2bq7di4u2lfl4qjka9-pd01.res.v6.highway.a1.net. [2001:871:227:33a8:f6a3:c58c:7641:e771]) by smtp.gmail.com with ESMTPSA id p188sm5722897wmp.30.2021.10.29.10.15.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Oct 2021 10:15:16 -0700 (PDT) Date: Fri, 29 Oct 2021 19:15:13 +0200 From: Bernhard Reutner-Fischer To: Jakub Jelinek Cc: rep.dot.nop@gmail.com, gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org, Bernhard Reutner-Fischer Subject: Re: [PATCH,Fortran 1/2] Add uop/name helpers Message-ID: <20211029191513.31ec4327@nbbrfq> In-Reply-To: <20211029111352.GZ304296@tucnak> References: <20211028235259.1411169-1-rep.dot.nop@gmail.com> <20211029111352.GZ304296@tucnak> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Oct 2021 17:15:20 -0000 On Fri, 29 Oct 2021 13:13:52 +0200 Jakub Jelinek wrote: > On Fri, Oct 29, 2021 at 01:52:58AM +0200, Bernhard Reutner-Fischer wrote: > > From: Bernhard Reutner-Fischer > > > > Introduce a helper to construct a user operator from a name and the > > reverse operation, i.e. a helper to construct a name from a user > > operator. > > > > Cc: Jakub Jelinek > > > > gcc/fortran/ChangeLog: > > > > 2017-10-29 Bernhard Reutner-Fischer > > > > * gfortran.h (gfc_get_uop_from_name, gfc_get_name_from_uop): Declare. > > * symbol.c (gfc_get_uop_from_name, gfc_get_name_from_uop): Define. > > * module.c (load_omp_udrs): Use them. > > --- > > gcc/fortran/gfortran.h | 2 ++ > > gcc/fortran/module.c | 21 +++------------------ > > gcc/fortran/symbol.c | 21 +++++++++++++++++++++ > > 3 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h > > index 9378b4b8a24..afe9f2354ee 100644 > > --- a/gcc/fortran/gfortran.h > > +++ b/gcc/fortran/gfortran.h > > @@ -3399,6 +3399,8 @@ void gfc_delete_symtree (gfc_symtree **, const char *); > > gfc_symtree *gfc_get_unique_symtree (gfc_namespace *); > > gfc_user_op *gfc_get_uop (const char *); > > gfc_user_op *gfc_find_uop (const char *, gfc_namespace *); > > +const char *gfc_get_uop_from_name (const char*); > > +const char *gfc_get_name_from_uop (const char*); > > Formatting, space between char and *. > > > --- a/gcc/fortran/symbol.c > > +++ b/gcc/fortran/symbol.c > > @@ -3044,6 +3044,27 @@ gfc_find_uop (const char *name, gfc_namespace *ns) > > return (st == NULL) ? NULL : st->n.uop; > > } > > > > +/* Given a name return a string usable as user operator name. */ > > +const char * > > +gfc_get_uop_from_name (const char* name) { > > Formatting, space before * rather than after it, { should go on next line. > Similarly later. Fixed the formatting. Sorry for my sloppiness.. > > But most importantly, I really don't like these helpers at all, they > unnecessarily allocate memory of the remaining duration of compilation, > and the second one even uses heap for temporary. Where do they allocate memory that remains during the rest of compilation? If we end up emitting the thing, then we will have it put into the stringpool anyway, so how's that bad? I did delete the allocated buffer after ht_lookup_with_hash copied the content so the temporary buffer in gfc_get_name_from_uop does not leak AFAICS. I can easily switch the second one to use XALLOCAVEC if you'd accept that? Ok? > Can't you just fix the real bug and keep the code as it was otherwise > (with XALLOCAVEC etc.)? > And, there should be a testcase... I tried to construct a testcase yesterday but failed. I took udr10.f90 and experimented with not using a derived type (because DERIVED || CLASS bypasses the failure to lookup st). I tried to move the module out to its own source to no avail and gave up late at night. Unrelated note: One thing that looked odd to my untrained eyes was in e.g. udr10.f90 where you write: !$omp parallel do reduction(+:j) reduction(.localadd.:k) do i = 1, 100 j = j .localadd. dl(i) k = k + dl(i * 2) which may of course be correct (even if + would be implemented by e.g. a twice_i procedure (and not addme like the user operator) but i'd have assumed the reduction oper to match the target var in the region, like !$omp parallel do reduction(.localadd.:j) reduction(+:k) But i admittedly know nothing about openmp syntax so it's certainly fine as written? PS: you have at least declare-simd-3.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } declare-target-2.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } and i think later on ! { dg-do compile { target skip-all-targets } } was added, presumably for this very purpose. thanks,