Re: [patch, libgfortran] Initailize some variable to get rid of nuisance warnings.
Pushed, thanks for feedback On 2/26/23 11:54 PM, Tobias Burnus wrote: Just side remarks, the 0 init in the patch is fine. On 27.02.23 03:53, Jerry D via Gcc-patches wrote: regarding PACK: since this is a bogus warning as the compiler does not realize that dim >= 1, wouldn't a gcc_assert (dim >= 1); Note: gcc_assert only exists in the compiler itself; in libgfortran, we use GFC_ASSERT or directly 'assert'. You could also use 'if (dim < 1) __builtin_unreachable();' – or since GCC 13: __attribute__((assume (dim >= 1))); Tobias PS: In Fortran, '-fopenmp-simd' plus '!$omp assume holds(dim>=0) ... !$omp end assume' (or !$omp ... + block/end block) can be used to denote such assumptions. '-fopenmp-simd' enables only those bits of OpenMP that do not require any library support (no libgomp, no pthreads), contrary to '-fopenmp'. - 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
Re: [PATCH] Fortran: fix corner case of IBITS intrinsic [PR108937]
On Mon, Feb 27, 2023 at 09:54:38PM +0100, Harald Anlauf via Fortran wrote: > > as found by the reporter, the result of the intrinsic IBITS > differed from other compilers (e.g. Intel, NAG) for the corner > case that the LEN argument was equal to BIT_SIZE(I), which is > explicitly allowed by the standard. > > We actually had an inconsistency for this case between > code generated by the frontend and compile-time simplified > expressions. > > The reporter noticed that this is related to a restriction in > gcc that requires that shift widths shall be smaller than the > bit sizes, and we already special case this for ISHFT. > It makes sense to use the same special casing for IBITS. > > Attached patch fixes this and regtests on x86_64-pc-linux-gnu. > > OK for mainline? Yes. Good catch on comparison with simplification, which I failed to consider last night. > This issue has been there for ages. Shall this be backported > or left in release branches as is? As always, backporting is up to you and your bandwidth. Bring the the run-time result and simplification into agreement suggests that a back port is a good thing. -- Steve
[PATCH] Fortran: fix corner case of IBITS intrinsic [PR108937]
Dear all,
as found by the reporter, the result of the intrinsic IBITS
differed from other compilers (e.g. Intel, NAG) for the corner
case that the LEN argument was equal to BIT_SIZE(I), which is
explicitly allowed by the standard.
We actually had an inconsistency for this case between
code generated by the frontend and compile-time simplified
expressions.
The reporter noticed that this is related to a restriction in
gcc that requires that shift widths shall be smaller than the
bit sizes, and we already special case this for ISHFT.
It makes sense to use the same special casing for IBITS.
Attached patch fixes this and regtests on x86_64-pc-linux-gnu.
OK for mainline?
This issue has been there for ages. Shall this be backported
or left in release branches as is?
Thanks,
Harald
From 6844c5ecb271e091a8c913903a79eac932cf5f76 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Mon, 27 Feb 2023 21:37:11 +0100
Subject: [PATCH] Fortran: fix corner case of IBITS intrinsic [PR108937]
gcc/fortran/ChangeLog:
PR fortran/108937
* trans-intrinsic.cc (gfc_conv_intrinsic_ibits): Handle corner case
LEN argument of IBITS equal to BITSIZE(I).
gcc/testsuite/ChangeLog:
PR fortran/108937
* gfortran.dg/ibits_2.f90: New test.
---
gcc/fortran/trans-intrinsic.cc| 10 +
gcc/testsuite/gfortran.dg/ibits_2.f90 | 32 +++
2 files changed, 42 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/ibits_2.f90
diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc
index 21eeb12ca89..3cce9c0166e 100644
--- a/gcc/fortran/trans-intrinsic.cc
+++ b/gcc/fortran/trans-intrinsic.cc
@@ -6638,6 +6638,7 @@ gfc_conv_intrinsic_ibits (gfc_se * se, gfc_expr * expr)
tree type;
tree tmp;
tree mask;
+ tree num_bits, cond;
gfc_conv_intrinsic_function_args (se, expr, args, 3);
type = TREE_TYPE (args[0]);
@@ -6678,8 +6679,17 @@ gfc_conv_intrinsic_ibits (gfc_se * se, gfc_expr * expr)
"in intrinsic IBITS", tmp1, tmp2, nbits);
}
+ /* The Fortran standard allows (shift width) LEN <= BIT_SIZE(I), whereas
+ gcc requires a shift width < BIT_SIZE(I), so we have to catch this
+ special case. See also gfc_conv_intrinsic_ishft (). */
+ num_bits = build_int_cst (TREE_TYPE (args[2]), TYPE_PRECISION (type));
+
mask = build_int_cst (type, -1);
mask = fold_build2_loc (input_location, LSHIFT_EXPR, type, mask, args[2]);
+ cond = fold_build2_loc (input_location, GE_EXPR, logical_type_node, args[2],
+ num_bits);
+ mask = fold_build3_loc (input_location, COND_EXPR, type, cond,
+ build_int_cst (type, 0), mask);
mask = fold_build1_loc (input_location, BIT_NOT_EXPR, type, mask);
tmp = fold_build2_loc (input_location, RSHIFT_EXPR, type, args[0], args[1]);
diff --git a/gcc/testsuite/gfortran.dg/ibits_2.f90 b/gcc/testsuite/gfortran.dg/ibits_2.f90
new file mode 100644
index 000..2af5542d764
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/ibits_2.f90
@@ -0,0 +1,32 @@
+! { dg-do run }
+! { dg-additional-options "-fcheck=bits" }
+! PR fortran/108937 - Intrinsic IBITS(I,POS,LEN) fails when LEN equals
+! to BIT_SIZE(I)
+! Contributed by saitofuy...@jamstec.go.jp
+
+program test_bits
+ implicit none
+ integer, parameter :: KT = kind (1)
+ integer, parameter :: lbits = bit_size (0_KT)
+ integer(kind=KT) :: x, y0, y1
+ integer(kind=KT) :: p, l
+
+ x = -1
+ p = 0
+ do l = 0, lbits
+ y0 = ibits (x, p, l)
+ y1 = ibits_1(x, p, l)
+ if (y0 /= y1) then
+print *, l, y0, y1
+stop 1+l
+ end if
+ end do
+contains
+ elemental integer(kind=KT) function ibits_1(I, POS, LEN) result(n)
+!! IBITS(I, POS, LEN) = (I >> POS) & ~((~0) << LEN)
+implicit none
+integer(kind=KT),intent(in) :: I
+integer, intent(in) :: POS, LEN
+n = IAND (ISHFT(I, - POS), NOT(ISHFT(-1_KT, LEN)))
+ end function ibits_1
+end program test_bits
--
2.35.3
[Patch,v3] Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings
And another re-diff for GCC 13/mainline, updating gcc/testsuite/ (The last change is related to the "[OG12,committed] Update dg-dump-scan for ..." discussion + OG12 https://gcc.gnu.org/g:e4de87a2309 / https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612871.html ) On 23.02.23 17:42, Tobias Burnus wrote: On 21.02.23 12:57, Tobias Burnus wrote: This patch moves some generic code for Fortran out of gimplify.cc to trans-openmp.cc and fixes several issues related to mapping. Tested with nvptx offloading. OK for mainline? Tobias Caveats: Besides the issues shown in the comment-out code, there remains also an issue with implicit mapping - at least for deferred-length strings, but I wouldn't be surprised if - at least depending on the used 'defaultmap' value (e.g. 'alloc') - there are also issues with array descriptors. Note: Regarding the declare target check for mapping: Without declare target, my assumption is that the hidden length variable will get implicitly mapped if needed. Independent of deferred-length or not, there is probably an issue with 'defaultmap(none)' and the hidden variable. - In any case, I prefer to defer all those issues to later (by having them captured in one/several PR). Tobias PS: This patch is a follow up to [Patch] Fortran/OpenMP: Fix DT struct-component with 'alloc' and array descr https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604887.html which fixed part of the problems. But as discussed on IRC, it did treat 'alloc' as special and missed some other map types. - In addition, this patch has a much extended test coverage and fixes some more issues found that way. - 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 Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings Previously, array descriptors might have been mapped as 'alloc' instead of 'to' for 'alloc', not updating the array bounds. The 'alloc' could also appear for 'data exit', failing with a libgomp assert. In some cases, either array descriptors or deferred-length string's length variable was not mapped. And, finally, some offset calculations with array-sections mappings went wrong. The testcases contain some comment-out tests which require follow-up work and for which PR exist. Those mostly relate to deferred-length strings which have several issues beyong OpenMP support. gcc/fortran/ChangeLog: * trans-decl.cc (gfc_get_symbol_decl): Add attributes such as 'declare target' also to hidden artificial variable for deferred-length character variables. * trans-openmp.cc (gfc_trans_omp_array_section, gfc_trans_omp_clauses, gfc_trans_omp_target_exit_data): Improve mapping of array descriptors and deferred-length string variables. gcc/ChangeLog: * gimplify.cc (gimplify_scan_omp_clauses): Remove Fortran special case. libgomp/ChangeLog: * testsuite/libgomp.fortran/target-enter-data-3.f90: Uncomment 'target exit data'. * testsuite/libgomp.fortran/target-enter-data-4.f90: New test. * testsuite/libgomp.fortran/target-enter-data-5.f90: New test. * testsuite/libgomp.fortran/target-enter-data-6.f90: New test. * testsuite/libgomp.fortran/target-enter-data-7.f90: New test. gcc/testsuite/ * gfortran.dg/goacc/finalize-1.f: Update dg-tree; shows a fix for 'finalize' as a ptr is now 'delete' instead of 'release'. * gfortran.dg/gomp/pr78260-2.f90: Likewise as elem-size calc moved to if (allocated) block * gfortran.dg/gomp/target-exit-data.f90: Likewise as a var is now a replaced by a MEM< _25 > expression. gcc/fortran/trans-decl.cc | 2 + gcc/fortran/trans-openmp.cc| 323 gcc/gimplify.cc| 43 +- gcc/testsuite/gfortran.dg/goacc/finalize-1.f | 4 +- gcc/testsuite/gfortran.dg/gomp/pr78260-2.f90 | 6 +- .../gfortran.dg/gomp/target-exit-data.f90 | 4 +- .../libgomp.fortran/target-enter-data-3.f90| 2 +- .../libgomp.fortran/target-enter-data-4.f90| 540 + .../libgomp.fortran/target-enter-data-5.f90| 540 + .../libgomp.fortran/target-enter-data-6.f90| 392 +++ .../libgomp.fortran/target-enter-data-7.f90| 78 +++ 11 files changed, 1792 insertions(+), 142 deletions(-) diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc index 474920966ec..c12eedfff1b 100644 --- a/gcc/fortran/trans-decl.cc +++ b/gcc/fortran/trans-decl.cc @@ -1824,6 +1824,8 @@ gfc_get_symbol_decl (gfc_symbol * sym) /* Add attributes to variables. Functions are handled elsewhere. */ attributes = add_attributes_to_decl (sym->attr, NULL_TREE); decl_attributes (, attributes, 0); + if (sym->ts.deferred) +decl_attributes (, attributes, 0); /* Symbols
[OG12,committed] Update dg-dump-scan for ... (was: [Patch] Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings)
Hi Thomas,
On 25.02.23 10:11, Thomas Schwinge wrote:
Do to the scan patterns need adjusting, or is something wrong?
The former. Regarding:
* gfortran.dg/goacc/finalize-1.f - for 'acc exit data':
!$ACC EXIT DATA FINALIZE DELETE (del_f_p(2:5))
Here, 'map\\(to:del_f_p [pointer set]' changed to 'map(release:del_f_p'
By itself, this is handled identically in libgomp. However, there is also
a 'finalize' - which change original-dump's 'release' to 'delete' in the
gimple dump. 'delete' is handled differently in terms of the refcount.
→ I believe the patch actually fixed an OpenACC issue by also force-unmapping
the descriptor and not only the pointer target.
* gfortran.dg/gomp/pr78260-2.f90
Here, the '* 4' multiplication moved from the expression, shown in the 'to'
clause to the expression evalulation.
Reasons: To work properly with deferred-length strings (first, to take the
current value and not some saved expr and to avoid issues when the var is
unallocated - especially with absent 'optional' variables.)
Side remark: On OG12 there are too many FAIL, compared to mainline. That does
not have anything to do with the items above - but it still makes working
with OG12 harder. I hope that OG13 will have fewer fails.
Tobias
PS: --word-diff for 'finalize-1.f':
original: [-map\\(to:del_f_p \\\[pointer set, len:-]{+map\\(release:del_f_p
\\\[len:+}
gimple: [-map\\(to:del_f_p \\\[pointer set, len:-]{+map\\(delete:del_f_p
\\\[len:+}
and for pr78260-2.f90, the "len: " changed (twice) as in
[-D.\[0-9\]+ \\* 4\\\]\\)-]{+D.\[0-9\]+\\\]\\)+}
and there is now additionally the following to ensure the '* 4' it not lost:
! { dg-final { scan-tree-dump-times "D\\.\[0-9\]+ = D\\.\[0-9\]+ \\* 4;" 2
"original" } }
-
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
commit e4de87a2309bb6eecc9d4a391e4454b6e51289c3
Author: Tobias Burnus
Date: Mon Feb 27 12:47:54 2023 +0100
Update dg-dump-scan for "Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings"
Follow-up to commit 55a18d4744258e3909568e425f9f473c49f9d13f
"Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings"
updating the dumps.
* For the goacc testcase, 'to' changed to 'release' and due to 'finally' then
to 'delete', which can be regarded as bugfix.
* For pr78260-2.f90, the calculation moved inside the 'if(...->data == NULL)'
block to handle deferred-string length vars better, esp. when 'optional'.
gcc/testsuite/:
* gfortran.dg/goacc/finalize-1.f: Update scan-tree-dump-times for
mapping changes.
* gfortran.dg/gomp/pr78260-2.f90: Likewise.
---
gcc/testsuite/ChangeLog.omp | 6 ++
gcc/testsuite/gfortran.dg/goacc/finalize-1.f | 4 ++--
gcc/testsuite/gfortran.dg/gomp/pr78260-2.f90 | 6 --
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/gcc/testsuite/ChangeLog.omp b/gcc/testsuite/ChangeLog.omp
index 98e41687633..d4217ccc71f 100644
--- a/gcc/testsuite/ChangeLog.omp
+++ b/gcc/testsuite/ChangeLog.omp
@@ -1,3 +1,9 @@
+2023-02-27 Tobias Burnus
+
+ * gfortran.dg/goacc/finalize-1.f: Update scan-tree-dump-times for
+ mapping changes.
+ * gfortran.dg/gomp/pr78260-2.f90: Likewise.
+
2023-02-23 Andrew Stubbs
Backport from mainline:
diff --git a/gcc/testsuite/gfortran.dg/goacc/finalize-1.f b/gcc/testsuite/gfortran.dg/goacc/finalize-1.f
index 1e5bf0ba1e6..23f0ffc627e 100644
--- a/gcc/testsuite/gfortran.dg/goacc/finalize-1.f
+++ b/gcc/testsuite/gfortran.dg/goacc/finalize-1.f
@@ -20,8 +20,8 @@
! { dg-final { scan-tree-dump-times "(?n)#pragma omp target oacc_exit_data map\\(delete:del_f \\\[len: \[0-9\]+\\\]\\) finalize$" 1 "gimple" } }
!$ACC EXIT DATA FINALIZE DELETE (del_f_p(2:5))
-! { dg-final { scan-tree-dump-times "(?n)#pragma acc exit data map\\(release:\\*\\(integer\\(kind=.\\)\\\[0:\\\] \\*\\) parm\\.0\\.data \\\[len: \[^\\\]\]+\\\]\\) map\\(to:del_f_p \\\[pointer set, len: \[0-9\]+\\\]\\) map\\(alloc:\\(integer\\(kind=1\\)\\\[0:\\\] \\* restrict\\) del_f_p\\.data \\\[pointer assign, bias: \\(.*int.*\\) parm\\.0\\.data - \\(.*int.*\\) del_f_p\\.data\\\]\\) finalize;$" 1 "original" } }
-! { dg-final { scan-tree-dump-times "(?n)#pragma omp target oacc_exit_data map\\(delete:MEM <\[^>\]+> \\\[\\(integer\\(kind=.\\)\\\[0:\\\] \\*\\)_\[0-9\]+\\\] \\\[len: \[^\\\]\]+\\\]\\) map\\(to:del_f_p \\\[pointer set, len: \[0-9\]+\\\]\\) map\\(alloc:del_f_p\\.data \\\[pointer assign, bias: \[^\\\]\]+\\\]\\) finalize$" 1 "gimple" } }
+! { dg-final { scan-tree-dump-times "(?n)#pragma acc exit data map\\(release:\\*\\(integer\\(kind=.\\)\\\[0:\\\] \\*\\) parm\\.0\\.data \\\[len: \[^\\\]\]+\\\]\\) map\\(release:del_f_p \\\[len: \[0-9\]+\\\]\\)
