Re: [Patch V2] Fix SLP PR58135.

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi Venkat,


On 23 May 2016 at 11:54, Kumar, Venkataramanan
<Venkataramanan.Kumar@amd.com> wrote:
> Hi Richard,
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guenther@gmail.com]
>> Sent: Thursday, May 19, 2016 4:08 PM
>> To: Kumar, Venkataramanan <Venkataramanan.Kumar@amd.com>
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [Patch V2] Fix SLP PR58135.
>>
>> On Wed, May 18, 2016 at 5:29 PM, Kumar, Venkataramanan
>> <Venkataramanan.Kumar@amd.com> wrote:
>> > Hi Richard,
>> >
>> >> -----Original Message-----
>> >> From: Richard Biener [mailto:richard.guenther@gmail.com]
>> >> Sent: Tuesday, May 17, 2016 5:40 PM
>> >> To: Kumar, Venkataramanan <Venkataramanan.Kumar@amd.com>
>> >> Cc: gcc-patches@gcc.gnu.org
>> >> Subject: Re: [Patch V2] Fix SLP PR58135.
>> >>
>> >> On Tue, May 17, 2016 at 1:56 PM, Kumar, Venkataramanan
>> >> <Venkataramanan.Kumar@amd.com> wrote:
>> >> > Hi Richard,
>> >> >
>> >> > I created the patch by passing -b option to git. Now the patch is
>> >> > more
>> >> readable.
>> >> >
>> >> > As per your suggestion I tried to fix the PR by splitting the SLP
>> >> > store group at
>> >> vector boundary after the SLP tree is built.
>> >> >
>> >> > Boot strap PASSED on x86_64.
>> >> > Checked the patch with check_GNU_style.sh.
>> >> >
>> >> > The gfortran.dg/pr46519-1.f test now does SLP vectorization. Hence
>> >> > it
>> >> generated 2 more vzeroupper.
>> >> > As recommended I adjusted the test case by adding
>> >> > -fno-tree-slp-vectorize
>> >> to make it as expected after loop vectorization.
>> >> >
>> >> > The following tests are now passing.
>> >> >
>> >> > ------ Snip-----
>> >> > Tests that now work, but didn't before:
>> >> >
>> >> > gcc.dg/vect/bb-slp-19.c -flto -ffat-lto-objects
>> >> > scan-tree-dump-times
>> >> > slp2 "basic block vectorized" 1
>> >> >
>> >> > gcc.dg/vect/bb-slp-19.c scan-tree-dump-times slp2 "basic block
>> >> > vectorized" 1
>> >> >
>> >> > New tests that PASS:
>> >> >
>> >> > gcc.dg/vect/pr58135.c (test for excess errors)
>> >> > gcc.dg/vect/pr58135.c -flto -ffat-lto-objects (test for excess
>> >> > errors)
>> >> >
>> >> > ------ Snip-----
>> >> >
>> >> > ChangeLog
>> >> >
>> >> > 2016-05-14  Venkataramanan Kumar
>> >> <Venkataramanan.kumar@amd.com>
>> >> >      PR tree-optimization/58135
>> >> >     * tree-vect-slp.c:  When group size is not multiple of vector size,
>> >> >      allow splitting of store group at vector boundary.
>> >> >
>> >> > Test suite  ChangeLog
>> >> > 2016-05-14  Venkataramanan Kumar
>> >> <Venkataramanan.kumar@amd.com>
>> >> >     * gcc.dg/vect/bb-slp-19.c:  Remove XFAIL.
>> >> >     * gcc.dg/vect/pr58135.c:  Add new.
>> >> >     * gfortran.dg/pr46519-1.f: Adjust test case.
>> >> >
>> >> > The attached patch Ok for trunk?
>> >>
>> >>
>> >> Please avoid the excessive vertical space around the vect_build_slp_tree
>> call.
>> > Yes fixed in the attached patch.
>> >>
>> >> +      /* Calculate the unrolling factor.  */
>> >> +      unrolling_factor = least_common_multiple
>> >> +                         (nunits, group_size) / group_size;
>> >> ...
>> >> +      else
>> >>         {
>> >>           /* Calculate the unrolling factor based on the smallest type.  */
>> >>           if (max_nunits > nunits)
>> >> -        unrolling_factor = least_common_multiple (max_nunits, group_size)
>> >> -                           / group_size;
>> >> +           unrolling_factor
>> >> +               = least_common_multiple (max_nunits,
>> >> + group_size)/group_size;
>> >>
>> >> please compute the "correct" unroll factor immediately and move the
>> >> "unrolling of BB required" error into the if() case by post-poning
>> >> the nunits < group_size check (and use max_nunits here).
>> >>
>> > Yes fixed in the attached patch.
>> >
>> >> +      if (is_a <bb_vec_info> (vinfo)
>> >> +         && nunits < group_size
>> >> +         && unrolling_factor != 1
>> >> +         && is_a <bb_vec_info> (vinfo))
>> >> +       {
>> >> +         dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
>> >> +                          "Build SLP failed: store group "
>> >> +                          "size not a multiple of the vector size "
>> >> +                          "in basic block SLP\n");
>> >> +         /* Fatal mismatch.  */
>> >> +         matches[nunits] = false;
>> >>
>> >> this is too pessimistic - you want to add the extra 'false' at
>> >> group_size / max_nunits * max_nunits.
>> > Yes fixed in attached patch.
>> >
>> >>
>> >> It looks like you leak 'node' in the if () path as well.  You need
>> >>
>> >>           vect_free_slp_tree (node);
>> >>           loads.release ();
>> >>
>> >> thus treat it as a failure case.
>> >
>> > Yes fixed. I added an else part before scalar_stmts.release call for the case
>> when SLP tree is not built. This avoids double freeing.
>> > Bootstrapped and reg tested on X86_64.
>> >

This patch is causing regressions on armeb:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/236591/report-build-info.html
The following fortran tests now fail at runtime:

  gfortran.dg/c_f_pointer_logical.f03   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution
test
  gfortran.dg/c_f_pointer_logical.f03   -O3 -g  execution test
  gfortran.dg/intrinsic_pack_1.f90   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution
test
  gfortran.dg/intrinsic_pack_1.f90   -O3 -g  execution test

Can you have a look?

Thanks,

Christophe.



>> > Ok for trunk ?
>>
>> +      /*Calculate the unrolling factor based on the smallest type.  */
>> +      unrolling_factor
>>
>> Space after /* missing.
>>
>> +      unrolling_factor
>> +       = least_common_multiple (max_nunits, group_size)/group_size;
>>
>> spaces before/after the '/'
>>
>> +      if (max_nunits > nunits
>> +         && unrolling_factor != 1
>> +         && is_a <bb_vec_info> (vinfo))
>>         {
>>           if (dump_enabled_p ())
>> -            dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
>> -                            "Build SLP failed: unrolling required in basic"
>> -                            " block SLP\n");
>> +             dump_printf_loc (MSG_MISSED_OPTIMIZATION,
>> +                              vect_location,
>> +                              "Build SLP failed: unrolling "
>> +                              "required in basic block SLP\n");
>>           vect_free_slp_tree (node);
>>           loads.release ();
>>           return false;
>>         }
>>
>> +      if (is_a <bb_vec_info> (vinfo)
>> +         && max_nunits < group_size
>> +         && unrolling_factor != 1)
>>
>> please merge these.  I think you have a not covered case of max_nunits ==
>> nunits, max_nunits > group_size.
>> So do
>>
>>     if (unrolling_factor != 1
>>         && is_a <...>)
>>      {
>>         if (max_nunits >= group_size)
>>           {
>>              first error
>>           }
>>         ... signal fatal mismatch instead...
>>      }
>>     else
>>      {
>>
>> Ok with that change.
>
> Thank you for the review. I updated and patch and up streamed it to trunk.
> Ref: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=236582
>
> Regards,
> Venkat.
>
>>
>> Thanks,
>> Richard.
>>
>> >>
>> >> Thanks,
>> >> Richard.
>> >>
>> >> > Regards,
>> >> > Venkat.
>> >> >
>> >
>> > Regards,
>> > Venkat.