Re: [PATCH] tree-optimization/99919 - bogus uninit diagnostic with bitfield guards
On Thu, Dec 8, 2022 at 12:07 PM Richard Biener via Fortran
wrote:
>
> For the testcase in this PR what fold-const.cc optimize_bit_field_compare
> does to bitfield against constant compares is confusing the uninit
> predicate analysis and it also makes SRA obfuscate instead of optimize
> the code. We've long had the opinion that those optimizations are
> premature but we do not have any replacement for the more complicated
> ones combining multiple bitfield tests. The following disables mangling
> the case of a single bitfield test against constants but preserving
> the existing diagnostic and optimization to a compile-time determined
> value.
>
> This requires silencing a bogus uninit diagnostic in the Fortran
> frontend which I've done in a minimal way, avoiding initializing
> the 40 byte symbol_attribute structure. There's several issues,
> one is the flag_coarrays is a global variable likely not CSEd
> to help the uninit predicate analysis, the other is us short-circuiting
> the flag_coarray == GFC_FCOARRAY_LIB && lhs_caf_attr.codimension
> accesses as both have no side-effects so the guard isn't effective.
> If the frontend folks are happy with this I can localize both
> lhs_caf_attr and rhs_caf_attr and copy out the two only flags
> tested by the code instead of the somewhat incomplete approach in
> the patch. Any opinions here?
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
Testing revealed this regresses
FAIL: c-c++-common/fold-masked-cmp-1.c -Wc++-compat
scan-assembler-times testn?b 2
FAIL: c-c++-common/fold-masked-cmp-2.c -Wc++-compat
scan-assembler-times testn?b 2
I filed PR108070 for the failure to optimize this on the RTL level
and put this change on hold :/
Richard.
> OK for the fortran parts?
>
> Thanks,
> Richard.
>
> PR tree-optimization/99919
> * fold-const.cc (optimize_bit_field_compare): Disable
> transforming the bitfield against constant compare optimization
> if the result is not statically determinable.
>
> gcc/fortran/
> * trans-expr.cc (gfc_trans_assignment_1): Split out
> lhs_codimension from lhs_caf_attr to avoid bogus uninit
> diagnostics.
>
> * gcc.dg/uninit-pr99919.c: New testcase.
> ---
> gcc/fold-const.cc | 37 +++
> gcc/fortran/trans-expr.cc | 6 +++--
> gcc/testsuite/gcc.dg/uninit-pr99919.c | 22
> 3 files changed, 30 insertions(+), 35 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/uninit-pr99919.c
>
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index cdfe3f50ae3..b72cc0a1d51 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -4559,7 +4559,6 @@ optimize_bit_field_compare (location_t loc, enum
> tree_code code,
> {
>poly_int64 plbitpos, plbitsize, rbitpos, rbitsize;
>HOST_WIDE_INT lbitpos, lbitsize, nbitpos, nbitsize;
> - tree type = TREE_TYPE (lhs);
>tree unsigned_type;
>int const_p = TREE_CODE (rhs) == INTEGER_CST;
>machine_mode lmode, rmode;
> @@ -4667,13 +4666,7 @@ optimize_bit_field_compare (location_t loc, enum
> tree_code code,
> }
>
>/* Otherwise, we are handling the constant case. See if the constant is
> too
> - big for the field. Warn and return a tree for 0 (false) if so. We do
> - this not only for its own sake, but to avoid having to test for this
> - error case below. If we didn't, we might generate wrong code.
> -
> - For unsigned fields, the constant shifted right by the field length
> should
> - be all zero. For signed fields, the high-order bits should agree with
> - the sign bit. */
> + big for the field. Warn and return a tree for 0 (false) if so. */
>
>if (lunsignedp)
> {
> @@ -4695,31 +4688,9 @@ optimize_bit_field_compare (location_t loc, enum
> tree_code code,
> }
> }
>
> - if (nbitpos < 0)
> -return 0;
> -
> - /* Single-bit compares should always be against zero. */
> - if (lbitsize == 1 && ! integer_zerop (rhs))
> -{
> - code = code == EQ_EXPR ? NE_EXPR : EQ_EXPR;
> - rhs = build_int_cst (type, 0);
> -}
> -
> - /* Make a new bitfield reference, shift the constant over the
> - appropriate number of bits and mask it with the computed mask
> - (in case this was a signed field). If we changed it, make a new one.
> */
> - lhs = make_bit_field_ref (loc, linner, lhs, unsigned_type,
> - nbitsize, nbitpos, 1, lreversep);
> -
> - rhs = const_binop (BIT_AND_EXPR,
> -const_binop (LSHIFT_EXPR,
> - fold_convert_loc (loc, unsigned_type, rhs),
> - size_int (lbitpos)),
> -mask);
> -
> - lhs = build2_loc (loc, code, compare_type,
> - build2 (BIT_AND_EXPR, unsigned_type, lhs, mask), rhs);
> - return lhs;
> + /* Otherwise do not prematurely optimize compares of bitfield members
> +
Re: [PATCH v5 2/4] OpenMP/OpenACC: Rework clause expansion and nested struct handling
On Wed, 7 Dec 2022 17:13:42 +0100
Tobias Burnus wrote:
> I think we need to distinguish:
>
>#pragma omp target enter data map(to: s.w[:10])
>
> from
>
>#pragma omp target map(tofrom: s.arr[:20])
> s.arr[0] = 5;
>
> As in the latter case 's' gets implicitly mapped and then applies to
> the base pointer 's.arr' of 's.arr[:20]'. While in the former case,
> only the pointee gets mapped without the pointer 's.arr' (and, hence,
> there is also no pointer attachment).
Here's an incremental patch that fixes the mapping behaviour in that
case. This is to be applied on top of the approved (but not committed)
parent patch:
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603792.html
and also the unreviewed patch posted here (ping?):
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607543.html
though it might actually make more sense to commit the three patches
squashed together.
Tested with offloading to NVPTX. OK?
Thanks,
Julian
commit abb1e04f9ef93221ecd4292f43cc1ea901843766
Author: Julian Brown
Date: Thu Dec 8 13:31:01 2022 +
OpenMP: implicitly map base pointer for array-section pointer components
Following from discussion in:
https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570075.html
and:
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608100.html
and also upstream OpenMP issue 342, this patch changes mapping for array
sections of pointer components on compute regions like this:
#pragma omp target map(s.ptr[0:10])
{
...use of 's'...
}
so the base pointer 's.ptr' is implicitly mapped, and thus pointer
attachment happens. This is subtly different in the "enter data"
case, e.g:
#pragma omp target enter data map(s.ptr[0:10])
if 's.ptr' (or the whole of 's') is not present on the target before
the directive is executed, the array section is copied to the target
but pointer attachment does *not* take place, since 's' (or 's.ptr')
is not mapped implicitly for "enter data".
To get a pointer attachment with "enter data", you can do, e.g:
#pragma omp target enter data map(s.ptr, s.ptr[0:10])
#pragma omp target
{
...implicit use of 's'...
}
That is, once the attachment has happened, implicit mapping of 's'
and uses of 's.ptr[...]' work correctly in the target region.
ChangeLog
2022-12-12 Julian Brown
gcc/
* gimplify.cc (omp_accumulate_sibling_list): Don't require
explicitly-mapped base pointer for compute regions.
gcc/testsuite/
* c-c++-comon/gomp/target-implicit-map-2.c: Update expected scan output.
libgomp/
* testsuite/libgomp.c-c++-common/target-implicit-map-2.c: Fix missing
"free".
* testsuite/libgomp.c-c++-common/target-implicit-map-3.c: New test.
* testsuite/libgomp.c-c++-common/target-map-zlas-1.c: New test.
* testsuite/libgomp.c/target-22.c: Remove explicit base pointer
mappings.
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 34cac30d7d92..a8dd298559e8 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -10617,6 +10617,7 @@ omp_accumulate_sibling_list (enum omp_region_type region_type,
poly_int64 cbitpos;
tree ocd = OMP_CLAUSE_DECL (grp_end);
bool openmp = !(region_type & ORT_ACC);
+ bool target = (region_type & ORT_TARGET) != 0;
tree *continue_at = NULL;
while (TREE_CODE (ocd) == ARRAY_REF)
@@ -10721,9 +10722,9 @@ omp_accumulate_sibling_list (enum omp_region_type region_type,
}
/* For OpenMP semantics, we don't want to implicitly allocate
- space for the pointer here. A FRAGILE_P node is only being
- created so that omp-low.cc is able to rewrite the struct
- properly.
+ space for the pointer here for non-compute regions (e.g. "enter
+ data"). A FRAGILE_P node is only being created so that
+ omp-low.cc is able to rewrite the struct properly.
For references (to pointers), we want to actually allocate the
space for the reference itself in the sorted list following the
struct node.
@@ -10731,6 +10732,7 @@ omp_accumulate_sibling_list (enum omp_region_type region_type,
mapping of the attachment point, but not otherwise. */
if (*fragile_p
|| (openmp
+ && !target
&& attach_detach
&& TREE_CODE (TREE_TYPE (ocd)) == POINTER_TYPE
&& !OMP_CLAUSE_ATTACHMENT_MAPPING_ERASED (grp_end)))
@@ -11043,6 +11045,7 @@ omp_accumulate_sibling_list (enum omp_region_type region_type,
if (*fragile_p
|| (openmp
+ && !target
&& attach_detach
&& TREE_CODE (TREE_TYPE (ocd)) == POINTER_TYPE
&& !OMP_CLAUSE_ATTACHMENT_MAPPING_ERASED (grp_end)))
diff --git a/gcc/testsuite/c-c++-common/gomp/target-implicit-map-2.c b/gcc/testsuite/c-c++-common/gomp/target-implicit-map-2.c
index 5ba
Re: [PATCH] Fortran: improve checking of assumed size array spec [PR102180]
Hi Steve,
Am 12.12.22 um 00:52 schrieb Steve Kargl via Gcc-patches:
On Sun, Dec 11, 2022 at 11:33:43PM +0100, Harald Anlauf via Fortran wrote:
Dear all,
the attached patch improves checking of array specs in two ways:
- bad assumed size array spec
- a bad first array element spec may already trigger an error,
leading to a more consistent behavior
Regtested on x86_64-pc-linux-gnu. OK for mainline?
OK with minor nit.
+ /* F2018:R821: "assumed-implied-spec is [ lower-bound : ] *". */
+ if (gfc_match (" : * ") == MATCH_YES)
+{
+ gfc_error ("A lower bound must precede colon in "
+"assumed size array specification at %C");
"assumed size" should likely be "assumed-size"
good point, I've fixed that.
I was a bit unhappy with the previously submitted patch,
as it tried to match ':' twice and gave an unfortunate locus
in the error message. I now chose to combine the adjacent
matches and to remember a more suitable locus for use with
the emitted error, see attached updated patch.
Committed as r13-4623-gcf5327b89ab610.
Thanks,
Harald
+ return AS_UNKNOWN;
+}
+
From cf5327b89ab610649c5faab78ea7907bb74b103c Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Sun, 11 Dec 2022 23:24:03 +0100
Subject: [PATCH] Fortran: improve checking of assumed-size array spec
[PR102180]
gcc/fortran/ChangeLog:
PR fortran/102180
* array.cc (match_array_element_spec): Add check for bad
assumed-implied-spec.
(gfc_match_array_spec): Reorder logic so that the first bad array
element spec may trigger an error.
gcc/testsuite/ChangeLog:
PR fortran/102180
* gfortran.dg/pr102180.f90: New test.
---
gcc/fortran/array.cc | 19 ---
gcc/testsuite/gfortran.dg/pr102180.f90 | 19 +++
2 files changed, 35 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr102180.f90
diff --git a/gcc/fortran/array.cc b/gcc/fortran/array.cc
index bbdb5b392fc..10d9e0c5354 100644
--- a/gcc/fortran/array.cc
+++ b/gcc/fortran/array.cc
@@ -489,7 +489,20 @@ match_array_element_spec (gfc_array_spec *as)
}
if (gfc_match_char (':') == MATCH_YES)
-return AS_DEFERRED;
+{
+ locus old_loc = gfc_current_locus;
+ if (gfc_match_char ('*') == MATCH_YES)
+ {
+ /* F2018:R821: "assumed-implied-spec is [ lower-bound : ] *". */
+ gfc_error ("A lower bound must precede colon in "
+ "assumed-size array specification at %L", &old_loc);
+ return AS_UNKNOWN;
+ }
+ else
+ {
+ return AS_DEFERRED;
+ }
+}
m = gfc_match_expr (upper);
if (m == MATCH_NO)
@@ -591,6 +604,8 @@ gfc_match_array_spec (gfc_array_spec **asp, bool match_dim, bool match_codim)
{
as->rank++;
current_type = match_array_element_spec (as);
+ if (current_type == AS_UNKNOWN)
+ goto cleanup;
/* Note that current_type == AS_ASSUMED_SIZE for both assumed-size
and implied-shape specifications. If the rank is at least 2, we can
@@ -600,8 +615,6 @@ gfc_match_array_spec (gfc_array_spec **asp, bool match_dim, bool match_codim)
if (as->rank == 1)
{
- if (current_type == AS_UNKNOWN)
- goto cleanup;
as->type = current_type;
}
else
diff --git a/gcc/testsuite/gfortran.dg/pr102180.f90 b/gcc/testsuite/gfortran.dg/pr102180.f90
new file mode 100644
index 000..cbf3e7299e7
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr102180.f90
@@ -0,0 +1,19 @@
+! { dg-do compile }
+! { dg-options "-fcoarray=lib" }
+! PR fortran/102180 - Improve checking of assumed size array spec
+
+subroutine s(x,y)
+ real :: x(0:*) ! legal
+ real :: y[0:*] ! legal
+end
+
+subroutine t(x,y)
+ real :: x(:*) ! { dg-error "A lower bound must precede colon" }
+ real :: y[:*] ! { dg-error "A lower bound must precede colon" }
+end
+
+subroutine u(x,y,z)
+ real :: x(2,*)
+ real :: y(2,2:*)
+ real :: z(2,:*) ! { dg-error "A lower bound must precede colon" }
+end
--
2.35.3
Re: [PATCH] Fortran: improve checking of assumed size array spec [PR102180]
On Mon, Dec 12, 2022 at 08:49:50PM +0100, Harald Anlauf via Fortran wrote:
>
> Committed as r13-4623-gcf5327b89ab610.
>
To be clear, I have no problems with this commit.
But, just a FYI, there is gfc_peek_ascii_char(),
which allows you to look at the next character
without having to keep track of the locus.
> +{
> + locus old_loc = gfc_current_locus;
> + if (gfc_match_char ('*') == MATCH_YES)
gfc_gobble_whitespace (); /* Can't remember if matching up to this
eats whitespace. */
if (gfc_peek_ascii_char () == '*')
> + {
> + /* F2018:R821: "assumed-implied-spec is [ lower-bound : ] *". */
> + gfc_error ("A lower bound must precede colon in "
> + "assumed-size array specification at %L", &old_loc);
> + return AS_UNKNOWN;
> + }
> + else
> + {
gfc_current_locus = old_loc; /* Is this needed? */
> + return AS_DEFERRED;
> + }
> +}
--
Steve
[PATCH] Fortran: NULL pointer dereference while parsing a function [PR107423]
Dear all,
here's another obvious patch by Steve which prevents a
NULL pointer dereference during parsing.
Regtested on x86_64-pc-linux-gnu.
Will commit to mainline within 24h unless there are comments.
Thanks,
Harald
From f23a5252ee086f9b78c44150d409e40a445c6928 Mon Sep 17 00:00:00 2001
From: Steve Kargl
Date: Mon, 12 Dec 2022 21:11:07 +0100
Subject: [PATCH] Fortran: NULL pointer dereference while parsing a function
[PR107423]
gcc/fortran/ChangeLog:
PR fortran/107423
* parse.cc (parse_spec): Avoid NULL pointer dereference when parsing
a function and an error occured.
gcc/testsuite/ChangeLog:
PR fortran/107423
* gfortran.dg/pr107423.f90: New test.
---
gcc/fortran/parse.cc | 2 +-
gcc/testsuite/gfortran.dg/pr107423.f90 | 18 ++
2 files changed, 19 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr107423.f90
diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
index cdae43fa1fd..bc2b2188eea 100644
--- a/gcc/fortran/parse.cc
+++ b/gcc/fortran/parse.cc
@@ -4015,7 +4015,7 @@ parse_spec (gfc_statement st)
gfc_symbol* proc = gfc_current_ns->proc_name;
gcc_assert (proc);
- if (proc->result->ts.type == BT_UNKNOWN)
+ if (proc->result && proc->result->ts.type == BT_UNKNOWN)
function_result_typed = true;
}
diff --git a/gcc/testsuite/gfortran.dg/pr107423.f90 b/gcc/testsuite/gfortran.dg/pr107423.f90
new file mode 100644
index 000..9ae64c94ae0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr107423.f90
@@ -0,0 +1,18 @@
+! { dg-do compile }
+! { dg-options "-std=f95" }
+! PR fortran/107423 - ICE in parse_spec
+! Contributed by G.Steinmetz
+
+program p
+ type t(k)
+ integer, kind :: k ! { dg-error "Fortran 2003" }
+ integer :: a
+ end type
+contains
+ function f()
+type(t(4)), allocatable :: x ! { dg-error "Invalid character" }
+allocate (t(4) :: x) ! { dg-error "cannot be used" }
+ end ! { dg-error "END" }
+end ! { dg-error "END" }
+
+! { dg-prune-output "Unexpected end of file" }
--
2.35.3
