Re: [PATCH 14/14] fortran: Pass pre-calculated class container argument [pr110618]
Hello, Le 14/07/2023 à 07:55, Paul Richard Thomas via Fortran a écrit : Hi Mikhail, This patch uses a field for gfc_se called class container, which is neither declared nor set as far as I can tell. These patches are dependent on my previous patchset for pr92178. Sorry, I didn't highlight that enough obviously. The field is declared and set here: https://gcc.gnu.org/pipermail/fortran/2023-July/059582.html https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624083.html It is additionally set in one more place in the next patch here: https://gcc.gnu.org/pipermail/fortran/2023-July/059583.html https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624084.html
[PATCH] [og13] OpenMP: Dimension ordering for array-shaping operator for C and C++
This patch fixes a bug in non-contiguous 'target update' operations using
the new array-shaping operator for C and C++, processing the dimensions
of the array the wrong way round during the OpenMP lowering pass.
Fortran was also incorrectly using the wrong ordering but the second
reversal in omp-low.cc made it produce the correct result.
The C and C++ bug only affected array shapes where the dimension sizes
are different ([X][Y]) - several existing tests used the same value
for both/all dimensions ([X][X]), which masked the problem. Only the
array dimensions (extents) are affected, not e.g. the indices, lengths
or strides for array sections.
This patch reverses the order used in both omp-low.cc and the Fortran
front-end, so the order should now be correct for all supported base
languages.
Tested with offloading to AMD GCN. I will apply (to og13) shortly.
2023-07-14 Julian Brown
gcc/fortran/
* trans-openmp.cc (gfc_trans_omp_arrayshape_type): Reverse dimension
ordering for created array type.
gcc/
* omp-low.cc (lower_omp_target): Reverse iteration over array
dimensions.
libgomp/
* testsuite/libgomp.c-c++-common/array-shaping-14.c: New test.
---
gcc/fortran/trans-openmp.cc | 2 +-
gcc/omp-low.cc| 6 ++--
.../libgomp.c-c++-common/array-shaping-14.c | 34 +++
3 files changed, 38 insertions(+), 4 deletions(-)
create mode 100644 libgomp/testsuite/libgomp.c-c++-common/array-shaping-14.c
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 6cb5340687e..6b9a0430eba 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -4271,7 +4271,7 @@ gfc_trans_omp_arrayshape_type (tree type, vec *dims)
{
gcc_assert (dims->length () > 0);
- for (int i = dims->length () - 1; i >= 0; i--)
+ for (unsigned i = 0; i < dims->length (); i++)
{
tree dim = fold_convert (sizetype, (*dims)[i]);
/* We need the index of the last element, not the array size. */
diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc
index c7706a5921f..ab2e4145ab2 100644
--- a/gcc/omp-low.cc
+++ b/gcc/omp-low.cc
@@ -14290,7 +14290,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
omp_context *ctx)
dims++;
}
- int tdim = tdims.length () - 1;
+ unsigned tdim = 0;
vec *vdim;
vec *vindex;
@@ -14365,7 +14365,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
omp_context *ctx)
nc = nc2;
}
- if (tdim >= 0)
+ if (tdim < tdims.length ())
{
/* We have an array shape -- use that to find the
total size of the data on the target to look up
@@ -14403,7 +14403,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
omp_context *ctx)
"for array");
dim = index = len = stride = error_mark_node;
}
- tdim--;
+ tdim++;
c = nc;
}
diff --git a/libgomp/testsuite/libgomp.c-c++-common/array-shaping-14.c
b/libgomp/testsuite/libgomp.c-c++-common/array-shaping-14.c
new file mode 100644
index 000..4ca6f794f93
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c-c++-common/array-shaping-14.c
@@ -0,0 +1,34 @@
+/* { dg-do run { target offload_device_nonshared_as } } */
+
+#include
+#include
+#include
+
+typedef struct {
+ int *ptr;
+} S;
+
+int main(void)
+{
+ S q;
+ q.ptr = (int *) calloc (9 * 11, sizeof (int));
+
+#pragma omp target enter data map(to: q.ptr, q.ptr[0:9*11])
+
+#pragma omp target
+ for (int i = 0; i < 9*11; i++)
+q.ptr[i] = i;
+
+#pragma omp target update from(([9][11]) q.ptr[3:3:2][1:4:3])
+
+ for (int j = 0; j < 9; j++)
+for (int i = 0; i < 11; i++)
+ if (j >= 3 && j <= 7 && ((j - 3) % 2) == 0
+ && i >= 1 && i <= 10 && ((i - 1) % 3) == 0)
+ assert (q.ptr[j * 11 + i] == j * 11 + i);
+ else
+ assert (q.ptr[j * 11 + i] == 0);
+
+#pragma omp target exit data map(release: q.ptr, q.ptr[0:9*11])
+ return 0;
+}
--
2.25.1
[PATCH] [og13] OpenMP: Enable c-c++-common/gomp/declare-mapper-3.c for C
This patch enables the c-c++-common/gomp/declare-mapper-3.c test for C.
This was seemingly overlooked in commit 393fd99c90e.
Tested with offloading to AMD GCN. I will apply (to og13) shortly.
2023-07-14 Julian Brown
gcc/testsuite/
* c-c++-common/gomp/declare-mapper-3.c: Enable for C.
---
gcc/testsuite/c-c++-common/gomp/declare-mapper-3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/c-c++-common/gomp/declare-mapper-3.c
b/gcc/testsuite/c-c++-common/gomp/declare-mapper-3.c
index 983d979d68c..e491bcd0ce6 100644
--- a/gcc/testsuite/c-c++-common/gomp/declare-mapper-3.c
+++ b/gcc/testsuite/c-c++-common/gomp/declare-mapper-3.c
@@ -1,4 +1,4 @@
-// { dg-do compile { target c++ } }
+// { dg-do compile }
// { dg-additional-options "-fdump-tree-gimple" }
#include
--
2.25.1
Re: [PATCH 00/14] fortran: Use precalculated class container for deallocation [PR110618]
Hi Mikael, I apologise for not being a bit faster with the review. I appreciate that you put a lot of effort into presenting the work in digestible chunks. Perhaps this is a personal perspective but I found it more difficult to absorb than reviewing a single patch. What do others think? That said, this is a big improvement to the finalization of variable expressions. I can also confirm that the composite patch applies cleanly and regtests without problems. Please either remove or uncomment the line: // gcc_assert (se.pre.head == NULL_TREE && se.post.head == NULL_TREE); I presume that it reflects some case where the assertion failed? If so, it might be prudent to retain the assertion especially in light of: gcc_assert (tmp_se.post.head == NULL_TREE); a bit further down. OK for trunk Thanks for all the work and the patches. Paul On Thu, 13 Jul 2023 at 19:40, Paul Richard Thomas wrote: > > Hi Mikael, > > All 14 patches apply cleanly to trunk, which is rebuilding right now > and will regtest this evening. > > I will review the composite patch tomorrow morning and will come back > to you as soon as I can. > > At first sight all is well; perhaps the commented out line can be > dispensed with? > > Many thanks for this. You are to be commended on your fortitude in > putting it all together. The result looks to be considerably neater > and more maintainable. > > If I recall correctly, Tobias was the author of much of this - any comments? > > Regards > > Paul > > > > On Thu, 13 Jul 2023 at 09:53, Mikael Morin via Fortran > wrote: > > > > Hello, > > > > the following patches are abot PR110618, a PR similar to PR92178 from which > > it is cloned. Both are about a problem of dedendencies between arguments, > > when one of them is associated to an allocatable intent(out) dummy, and thus > > deallocated in the process of argument association. > > > > PR110618 exposes a case where the data reference finalization code > > for one argument references deallocated data from another argument. > > The way I propose to fix this is similar to my recent patches for > > PR92178 [1,2] (and is dependent on them). Those patches try to use a data > > reference pointer precalculated at the beginning of the process instead of > > repeatedly evaluating an expression that becomes invalid at some point > > in the generated code. > > > > Unfortunately, the code for finalization is not prepared for this, as it > > only manipulates front-end expressions, whereas the precalculated > > pointer is available as middle-end's generic tree. > > > > These patches refactor the finalization code to ease the introduction > > of the forementioned pre-calculated class container pointer. Basically, > > four expressions are calculated to build the final procedure call: > > the final procedure pointer, the element size, the data reference > > (array) descriptor, and (optionally) the virtual table pointer. Each of > > the four is outlined stepwise to its own separate function in the > > following patches. This abstracts away the generation of these > > expressions and makes it easier to add one other way to generate them. > > This should also make the impact of the changes more > > visible, and regressions easier to spot. > > > > The main changes are the two last patches introducing an additional > > precalculated pointer argument in relevant functions and using them if > > set. Details are in the specific patches. > > > > Each patch has been bubble-bootstrapped and partially tested > > with RUNTESTFLAGS="dg.exp=*final*". > > The complete set has been fully tested on x86_64-pc-linux-gnu. > > OK for master? > > > > [1] https://gcc.gnu.org/pipermail/fortran/2023-July/059582.html > > [2] https://gcc.gnu.org/pipermail/fortran/2023-July/059583.html > > > > Mikael Morin (14): > > fortran: Outline final procedure pointer evaluation > > fortran: Outline element size evaluation > > fortran: Outline data reference descriptor evaluation > > fortran: Inline gfc_build_final_call > > fortran: Add missing cleanup blocks > > fortran: Reuse final procedure pointer expression > > fortran: Push element size expression generation close to its usage > > fortran: Push final procedure expr gen close to its one usage. > > fortran: Inline variable definition > > fortran: Remove redundant argument in get_var_descr > > fortran: Outline virtual table pointer evaluation > > fortran: Factor scalar descriptor generation > > fortran: Use pre-evaluated class container if available [PR110618] > > fortran: Pass pre-calculated class container argument [pr110618] > > > > gcc/fortran/trans-array.cc | 2 +- > > gcc/fortran/trans-expr.cc | 7 +- > > gcc/fortran/trans-stmt.cc | 3 +- > > gcc/fortran/trans.cc| 314 > > gcc/fortran/trans.h | 9 +- > > gcc/testsuite/gfortran.dg/intent_out_22.f90 | 37 +++ > > 6 files changed, 2
