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

[Richard Guenther <richard dot guenther at gmail dot com>]

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
>