This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [Patch V2] Fix SLP PR58135.
- From: "Kumar, Venkataramanan" <Venkataramanan dot Kumar at amd dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 23 May 2016 09:54:47 +0000
- Subject: RE: [Patch V2] Fix SLP PR58135.
- Authentication-results: sourceware.org; auth=none
- Authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=amd.com;
- References: <CY1PR1201MB10988F2DF32D1E853BCBC0EA8F480 at CY1PR1201MB1098 dot namprd12 dot prod dot outlook dot com> <CAFiYyc2yzmfD2ok-XdGD5d2_gmhJ9066fH4eSs7kSLHtM8pe8Q at mail dot gmail dot com> <CY1PR1201MB1098A879C8612260029E00CC8F490 at CY1PR1201MB1098 dot namprd12 dot prod dot outlook dot com> <CAFiYyc0f_fmeswU=4FEPPq8o5sv8BD5Bj46OOLtnmnWwGTBkJA at mail dot gmail dot com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:23
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.
> >
> > 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.