[Patch, Fortran] PR 41714: [OOP] ALLOCATE SOURCE= does not properly copy the value from SOURCE

[Richard Guenther]

On Sun, Oct 25, 2009 at 10:42 PM, Janus Weil <janus@gcc.gnu.org> wrote:
> 2009/10/25 Janus Weil <janus@gcc.gnu.org>:
>>>>>>>> It looks OK to me except:
>>>>>>>>
>>>>>>>>>        PR fortran/41714
>>>>>>>>>        * trans-expr.c (gfc_build_memcpy_call): Take care of the case that the
>>>>>>>>>        call to '__builtin_memcpy' is optimized away (replaced by a direct
>>>>>>>>>        assignment).
>>>>>>>>
>>>>>>>> How the heck does that work?  It comes out as a NOP_EXPR and yet it's
>>>>>>>> really an assignment..... Is that documented somewhere?
>>>>>>>
>>>>>>> That patch looks indeed dubious.  It tests for an implementation detail
>>>>>>> (the memcpy folder returns (void *) ({ *dst = *src; dst; })).  You should
>>>>>>> be able to unconditionally fold-convert to void_type_node as in the
>>>>>>> original code.  Instead tree_annotate_all_with_location should be fixed.
>>>>>>
>>>>>> Or rather the FE should not call this function - it assumes that the code
>>>>>> is already gimplified.
>>>>>
>>>>> Ok, so you mean one should instead just do the stuff which this
>>>>> function does, but without the extra checks? Like here:
>>>>>
>>>>> Index: gcc/fortran/trans.c
>>>>> ===================================================================
>>>>> --- gcc/fortran/trans.c (Revision 153538)
>>>>> +++ gcc/fortran/trans.c (Arbeitskopie)
>>>>> @@ -1282,7 +1282,11 @@ gfc_trans_code (gfc_code * code)
>>>>>       if (res != NULL_TREE && ! IS_EMPTY_STMT (res))
>>>>>        {
>>>>>          if (TREE_CODE (res) == STATEMENT_LIST)
>>>>> -           tree_annotate_all_with_location (&res, input_location);
>>>>> +           {
>>>>> +             tree_stmt_iterator i;
>>>>> +             for (i = tsi_start (res); !tsi_end_p (i); tsi_next (&i))
>>>>> +               SET_EXPR_LOCATION (tsi_stmt (i), input_location);
>>>>> +           }
>>>>>          else
>>>>>            SET_EXPR_LOCATION (res, input_location);
>>>>
>>>> No.  I think the above should just be dropped (as well as the other
>>>> call in the Fortran frontend).  The location should have been set
>>>> by the various stmt builders (like build_call_expr_loc in the
>>>> memcpy case).  For the folding of memcpy case
>>>> the folder will have distributed the locations appropriately.
>>>>
>>>> The middle-end function can then be removed completely (the Fortran
>>>> FE is the only caller).  A patch to do so is pre-approved.
>>>
>>> Alright. Regtesting the attached patch now.
>>
>> Regtesting was not successful, unfortunately:
>>
>> FAIL: gfortran.dg/gomp/appendix-a/a.24.1.f90  -O   (test for errors, line 20)
>> FAIL: gfortran.dg/gomp/appendix-a/a.24.1.f90  -O   (test for errors, line 14)
>> FAIL: gfortran.dg/gomp/appendix-a/a.24.1.f90  -O   (test for errors, line 20)
>> FAIL: gfortran.dg/gomp/appendix-a/a.24.1.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.1.f90  -O   (test for warnings, line 13)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.1.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.3.f90  -O   (test for warnings, line 10)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.3.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.4.f90  -O   (test for warnings, line 11)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.4.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.6.f90  -O   (test for warnings, line 9)
>> FAIL: gfortran.dg/gomp/appendix-a/a.35.6.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/block-1.f90  -O   (test for errors, line 5)
>> FAIL: gfortran.dg/gomp/block-1.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/crayptr3.f90  -O   (test for errors, line 19)
>> FAIL: gfortran.dg/gomp/crayptr3.f90  -O   (test for errors, line 20)
>> FAIL: gfortran.dg/gomp/crayptr3.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O   (test for errors, line 8)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O   (test for errors, line 10)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O   (test for errors, line 21)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O   (test for errors, line 22)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O   (test for errors, line 33)
>> FAIL: gfortran.dg/gomp/pr33439.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/sharing-1.f90  -O   (test for errors, line 12)
>> FAIL: gfortran.dg/gomp/sharing-1.f90  -O   (test for errors, line 24)
>> FAIL: gfortran.dg/gomp/sharing-1.f90  -O   (test for errors, line 25)
>> FAIL: gfortran.dg/gomp/sharing-1.f90  -O   (test for errors, line 26)
>> FAIL: gfortran.dg/gomp/sharing-1.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 12)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 16)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 57)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 58)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 64)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O   (test for errors, line 65)
>> FAIL: gfortran.dg/gomp/sharing-2.f90  -O  (test for excess errors)
>> FAIL: gfortran.dg/gomp/sharing-3.f90  -O   (test for errors, line 28)
>> FAIL: gfortran.dg/gomp/sharing-3.f90  -O   (test for errors, line 30)
>> FAIL: gfortran.dg/gomp/sharing-3.f90  -O   (test for errors, line 33)
>> FAIL: gfortran.dg/gomp/sharing-3.f90  -O   (test for errors, line 34)
>> FAIL: gfortran.dg/gomp/sharing-3.f90  -O  (test for excess errors)
>>
>>
>> It seems removing 'tree_annotate_all_with_location' in trans-openmp.c
>> was no good idea.
>
> Huh. I was assuming all of the above regressions were due to the hunk
> in trans-openmp.c. However, it turns out they are still present after
> removing it. Which means they must be due to the removal of
> 'tree_annotate_all_with_location' in trans.c.
>
> So, what to do? Are we back to
>
> Index: gcc/fortran/trans.c
> ===================================================================
> --- gcc/fortran/trans.c (Revision 153538)
> +++ gcc/fortran/trans.c (Arbeitskopie)
> @@ -1282,7 +1282,11 @@ gfc_trans_code (gfc_code * code)
>      if (res != NULL_TREE && ! IS_EMPTY_STMT (res))
>       {
>         if (TREE_CODE (res) == STATEMENT_LIST)
> -           tree_annotate_all_with_location (&res, input_location);
> +           {
> +             tree_stmt_iterator i;
> +             for (i = tsi_start (res); !tsi_end_p (i); tsi_next (&i))
> +               SET_EXPR_LOCATION (tsi_stmt (i), input_location);
> +           }
>         else
>           SET_EXPR_LOCATION (res, input_location);
>
> or is there a better option? (One alternative could be to set the
> location only for OpenMP cases, since all other things seem to work?)

I suggest to find out which expressions miss a proper location and fix
it where they are generated.

Richard.

> Cheers,
> Janus
>