| Message ID | 20160921145313.GB7282@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 21 Sep 2016, Jakub Jelinek wrote: > The simduid pass uses the cfun->has_simduid_loops flag to determine if it > needs to clean up any left-over GOMP_SIMD_* internal functions. > During inlining, we set the flag if we inline some loop with simduid, or if > we find GOMP_SIMD_ORDERED_* internal call, but the testcase shows that we > need to do that for the other GOMP_SIMD_* ifns too, because what we inline > might have turned the loop->simduid loops into non-loops (or they might not > exist from the beginning because simd region had noreturn body). I think this means that the comment at the declaration of has_simduid in function.h needs to be updated. I see that there are other places in the compiler that propagate the has_simduid property. Now that the property is tied to just the presence of GOMP_SIMD_xyz IFNs, won't those other places need changes too? I wonder why this property is propagated with such fine granularity in the first place. Wouldn't it be simpler and almost as efficient to propagate it on the function basis, i.e. when inlining A into B set it on B if A had it, without regard for whether uses in A have already been DCE'd? Thanks. Alexander
On Wed, 21 Sep 2016, Alexander Monakov wrote: > On Wed, 21 Sep 2016, Jakub Jelinek wrote: > > The simduid pass uses the cfun->has_simduid_loops flag to determine if it > > needs to clean up any left-over GOMP_SIMD_* internal functions. > > During inlining, we set the flag if we inline some loop with simduid, or if > > we find GOMP_SIMD_ORDERED_* internal call, but the testcase shows that we > > need to do that for the other GOMP_SIMD_* ifns too, because what we inline > > might have turned the loop->simduid loops into non-loops (or they might not > > exist from the beginning because simd region had noreturn body). > > I think this means that the comment at the declaration of has_simduid in > function.h needs to be updated. > > I see that there are other places in the compiler that propagate the has_simduid > property. Now that the property is tied to just the presence of GOMP_SIMD_xyz > IFNs, won't those other places need changes too? > > I wonder why this property is propagated with such fine granularity in the first > place. Wouldn't it be simpler and almost as efficient to propagate it on the > function basis, i.e. when inlining A into B set it on B if A had it, without > regard for whether uses in A have already been DCE'd? That would indeed simplify things but the patch is ok as-is with me as well. Thanks, Richard.
--- gcc/tree-inline.c.jj 2016-08-06 12:11:57.000000000 +0200 +++ gcc/tree-inline.c 2016-09-21 13:40:23.153732520 +0200 @@ -1644,11 +1644,19 @@ remap_gimple_stmt (gimple *stmt, copy_bo gimple_call_set_tail (call_stmt, false); if (gimple_call_from_thunk_p (call_stmt)) gimple_call_set_from_thunk (call_stmt, false); - if (gimple_call_internal_p (call_stmt) - && IN_RANGE (gimple_call_internal_fn (call_stmt), - IFN_GOMP_SIMD_ORDERED_START, - IFN_GOMP_SIMD_ORDERED_END)) - DECL_STRUCT_FUNCTION (id->dst_fn)->has_simduid_loops = true; + if (gimple_call_internal_p (call_stmt)) + switch (gimple_call_internal_fn (call_stmt)) + { + case IFN_GOMP_SIMD_LANE: + case IFN_GOMP_SIMD_VF: + case IFN_GOMP_SIMD_LAST_LANE: + case IFN_GOMP_SIMD_ORDERED_START: + case IFN_GOMP_SIMD_ORDERED_END: + DECL_STRUCT_FUNCTION (id->dst_fn)->has_simduid_loops = true; + break; + default: + break; + } } /* Remap the region numbers for __builtin_eh_{pointer,filter}, --- gcc/testsuite/gfortran.dg/gomp/pr77665.f90.jj 2016-09-21 13:50:34.304948175 +0200 +++ gcc/testsuite/gfortran.dg/gomp/pr77665.f90 2016-09-21 13:51:01.163605047 +0200 @@ -0,0 +1,18 @@ +! PR fortran/77665 +! { dg-do compile } +! { dg-additional-options "-O2" } + +program pr77665 + type t + integer :: a = 0 + end type + type(t) :: x + integer :: i + !$omp declare reduction (+:t: omp_out%a = omp_out%a + omp_in%a) + !$omp simd reduction(+:x) + do i = 1, 8 + if (abs(i) < 5) call abort + x%a = x%a + 1 + end do + print *, x%a +end