| Message ID | CAHqFgjW=Gs4kOAwNb=Hciqd3v9S=YhsLeHPSWLAh=XQobvV+yA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 15.09.2014 19:18, Alessandro Fanfarillo wrote: > New patch after the update. > > > 2014-09-09 0:30 GMT-06:00 Tobias Burnus<burnus@net-b.de>: >> >I think I'd prefer the following patch, which avoids a temporary if none is >> >required. "value" is a pointer if the kind is the same (see kind check >> >before) and if it is not a literal. Otherwise, it isn't a pointer and one >> >needs to generate a temporary. >> > >> >I do not quite understand why the current check doesn't work as both are >> >integer(kind=4) but for some reasons one has a variant. >> > >> >Additionally, I wonder whether one should add a test case – one probably >> >should do – and of which kind (run test + fdump-tree-original?). >> >> >@@ -8398,3 +8398,3 @@ conv_intrinsic_atomic_op (gfc_code *code) >> >- if (TREE_TYPE (TREE_TYPE (atom)) != TREE_TYPE (TREE_TYPE (value))) >> >+ if (!POINTER_TYPE_P (TREE_TYPE (value))) > 2014-09-15 Alessandro Fanfarillo<fanfarillo.gcc@gmail.com> > Tobias Burnus<burnus@net-b.de> > > * trans-intrinsic.c (conv_intrinsic_atomic_op): > Check for indirect reference for caf_atomic_op value. > > diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c > index a13b113..2d7241a 100644 > --- a/gcc/fortran/trans-intrinsic.c > +++ b/gcc/fortran/trans-intrinsic.c > @@ -8396,9 +8396,11 @@ conv_intrinsic_atomic_op (gfc_code *code) > else > image_index = integer_zero_node; > > - if (TREE_TYPE (TREE_TYPE (atom)) != TREE_TYPE (TREE_TYPE (value))) > + if (!POINTER_TYPE_P (TREE_TYPE (value))) > { > tmp = gfc_create_var (TREE_TYPE (TREE_TYPE (atom)), "value"); > + if (POINTER_TYPE_P (TREE_TYPE (value))) > + value = build_fold_indirect_ref_loc (input_location, value); The second part makes no sense: If "value" is not a pointer (which is the first condition), it can never be a pointer (second condition). Otherwise, the patch is okay. The reason I hadn't committed it myself was that I wanted to include a test case; I was wondering whether it should be a run test – or a -fdump-tree-original + scan-tree test – or both. Can you create a test case? Tobias
In attachment a test case which fails with the current gcc-trunk version but works when the patch is applied. coarray_35.f90 is my attempt to write a gcc test case. The problem is related with atomic_add. 2014-09-15 12:55 GMT-06:00 Tobias Burnus <burnus@net-b.de>: > On 15.09.2014 19:18, Alessandro Fanfarillo wrote: > > New patch after the update. > > > 2014-09-09 0:30 GMT-06:00 Tobias Burnus <burnus@net-b.de>: > >> I think I'd prefer the following patch, which avoids a temporary if none >> is >> required. "value" is a pointer if the kind is the same (see kind check >> before) and if it is not a literal. Otherwise, it isn't a pointer and one >> needs to generate a temporary. >> >> I do not quite understand why the current check doesn't work as both are >> integer(kind=4) but for some reasons one has a variant. >> >> Additionally, I wonder whether one should add a test case – one probably >> should do – and of which kind (run test + fdump-tree-original?). > >> @@ -8398,3 +8398,3 @@ conv_intrinsic_atomic_op (gfc_code *code) >> - if (TREE_TYPE (TREE_TYPE (atom)) != TREE_TYPE (TREE_TYPE (value))) >> + if (!POINTER_TYPE_P (TREE_TYPE (value))) > > 2014-09-15 Alessandro Fanfarillo <fanfarillo.gcc@gmail.com> > Tobias Burnus <burnus@net-b.de> > > * trans-intrinsic.c (conv_intrinsic_atomic_op): > Check for indirect reference for caf_atomic_op value. > > diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c > index a13b113..2d7241a 100644 > --- a/gcc/fortran/trans-intrinsic.c > +++ b/gcc/fortran/trans-intrinsic.c > @@ -8396,9 +8396,11 @@ conv_intrinsic_atomic_op (gfc_code *code) > else > image_index = integer_zero_node; > > - if (TREE_TYPE (TREE_TYPE (atom)) != TREE_TYPE (TREE_TYPE (value))) > + if (!POINTER_TYPE_P (TREE_TYPE (value))) > { > tmp = gfc_create_var (TREE_TYPE (TREE_TYPE (atom)), "value"); > + if (POINTER_TYPE_P (TREE_TYPE (value))) > + value = build_fold_indirect_ref_loc (input_location, value); > > > The second part makes no sense: If "value" is not a pointer (which is the > first condition), it can never be a pointer (second condition). > > Otherwise, the patch is okay. The reason I hadn't committed it myself was > that I wanted to include a test case; I was wondering whether it should be a > run test – or a -fdump-tree-original + scan-tree test – or both. > > Can you create a test case? > > Tobias
On 15.09.2014 22:23, Alessandro Fanfarillo wrote: > In attachment a test case which fails with the current gcc-trunk > version but works when the patch is applied. coarray_35.f90 is my > attempt to write a gcc test case. > The problem is related with atomic_add. Well, if it is a "dg-do compile" test, it won't exercise the issue: Even without the patch, it was compiling. It should either be a run test (dg-do run) – and then under gfortran.dg/coarray/ - which will automatically link libcaf_single to it. (And do another run with -fcoarray=single). – Or you have to additionally use -fdump-tree-original and scan for the strings, using "dg-final { scan-tree-dump-times ... or scan-tree-dump-not. See other test cases there. I think ATOMIC_ADD should also fail without the patch, i.e. the run test really would test whether it works. Thus, that might be the simpler option. Or you do both – but the dump one shouldn't be under gfortran.dg/coarray/ as that is also run with -fcoarray=single, but under gfortran.dg/ directly. Tobias
diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c index a13b113..2d7241a 100644 --- a/gcc/fortran/trans-intrinsic.c +++ b/gcc/fortran/trans-intrinsic.c @@ -8396,9 +8396,11 @@ conv_intrinsic_atomic_op (gfc_code *code) else image_index = integer_zero_node; - if (TREE_TYPE (TREE_TYPE (atom)) != TREE_TYPE (TREE_TYPE (value))) + if (!POINTER_TYPE_P (TREE_TYPE (value))) { tmp = gfc_create_var (TREE_TYPE (TREE_TYPE (atom)), "value"); + if (POINTER_TYPE_P (TREE_TYPE (value))) + value = build_fold_indirect_ref_loc (input_location, value); gfc_add_modify (&block, tmp, fold_convert (TREE_TYPE (tmp), value)); value = gfc_build_addr_expr (NULL_TREE, tmp); }