Re: [PATCH][vectorizer][2/2] Hook up mult synthesis logic into vectorisation of mult-by-constant

[Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>]

Hi Richard,

> On Thu, 14 Jul 2016, Kyrill Tkachov wrote:
>
>> Hi all,
>> 
>> On 07/07/16 17:16, Kyrill Tkachov wrote:
>> > 
>> > On 06/07/16 13:40, Kyrill Tkachov wrote:
>> > > 
>> > > On 06/07/16 13:31, Rainer Orth wrote:
>> > > > Hi Kyrill,
>> > > > 
>> > > > > On 05/07/16 12:24, Rainer Orth wrote:
>> > > > > > Marc Glisse <marc.glisse@inria.fr> writes:
>> > > > > > 
>> > > > > > > On Tue, 5 Jul 2016, Kyrill Tkachov wrote:
>> > > > > > > 
>> > > > > > > > As for testing I've bootstrapped and tested the patch on aarch64
>> > > > > > > > and
>> > > > > > > > x86_64 with synth_shift_p in vect_synth_mult_by_constant hacked
>> > > > > > > > to be
>> > > > > > > > always true to exercise the paths that synthesize the shift by
>> > > > > > > > additions. Marc, could you test this on the sparc targets you
>> > > > > > > > care about?
>> > > > > > > I don't have access to Sparc, you want Rainer here (added in Cc:).
>> > > > > > As is, the patch doesn't even build on current mainline:
>> > > > > > 
>> > > > > > /vol/gcc/src/hg/trunk/local/gcc/tree-vect-patterns.c:2151:56: error:
>> > > > > > 'mult_variant' has not been declared
>> > > > > >    target_supports_mult_synth_alg (struct algorithm *alg,
>> > > > > > mult_variant var,
>> > > > > > ^
>> > > > > > 
>> > > > > > enum mult_variant is only declared in expmed.c.
>> > > > > Ah, sorry I forgot to mention that this is patch 2/2.
>> > > > > It requires https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01144.html
>> > > > > which
>> > > > > is already approved
>> > > > > but I was planning to commit it together with this one.
>> > > > > Can you please try applying
>> > > > > https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01144.html
>> > > > > as well as this?
>> > > > sure, that did the trick.  A sparc-sun-solaris2.12 bootstrap revealed
>> > > > that the patch fixes PR tree-optimization/70923 (you should mention that
>> > > > in the ChangeLog or close it as a duplicate), with the same caveat as
>> > > > about Marc's latest patch for that:
>> > > 
>> > > Thanks! Much appreciated.
>> > > 
>> > > > +FAIL: gcc.dg/vect/vect-iv-9.c -flto -ffat-lto-objects
>> > > > scan-tree-dump-times vect "vectorized 1 loops" 1
>> > > > +FAIL: gcc.dg/vect/vect-iv-9.c scan-tree-dump-times vect "vectorized 1
>> > > > loops" 1
>> > > > 
>> > > > The message appears twice, not once, on sparc, so the testcase should be
>> > > > updated to accomodate that.
>> > > 
>> > > Ok.
>> > > 
>> > > > Besides, the new testcase FAILs:
>> > > > 
>> > > > +FAIL: gcc.dg/vect/pr65951.c -flto -ffat-lto-objects
>> > > > scan-tree-dump-times vect "vectorized 1 loops" 2
>> > > > +FAIL: gcc.dg/vect/pr65951.c scan-tree-dump-times vect "vectorized 1
>> > > > loops" 2
>> > > > 
>> > > > The dump contains
>> > > > 
>> > > > gcc.dg/vect/pr65951.c:14:3: note: not vectorized: no vectype for stmt:
>> > > > _4 = *_3;
>> > > > gcc.dg/vect/pr65951.c:12:1: note: vectorized 0 loops in function.
>> > > > gcc.dg/vect/pr65951.c:21:3: note: not vectorized: no vectype for stmt:
>> > > > _4 = *_3;
>> > > > gcc.dg/vect/pr65951.c:19:1: note: vectorized 0 loops in function.
>> > > > gcc.dg/vect/pr65951.c:55:15: note: not vectorized: control flow in loop.
>> > > > gcc.dg/vect/pr65951.c:46:3: note: not vectorized: loop contains function
>> > > > calls or data references that cannot be analyzed
>> > > > gcc.dg/vect/pr65951.c:41:15: note: not vectorized: control flow in loop.
>> > > > gcc.dg/vect/pr65951.c:32:3: note: not vectorized: loop contains function
>> > > > calls or data references that cannot be analyzed
>> > > > gcc.dg/vect/pr65951.c:26:1: note: vectorized 0 loops in function.
>> > > 
>> > > I see. I suppose SPARC doesn't have vector shifts operating on 64-bit
>> > > data?
>> > > I believe the testcase should be updated to use just "int" arrays rather
>> > > than "long long".
>> > > 
>> > > I'll respin the testcases
>> > 
>> > Ok, here's the patch with pr65951.c updated to use int rather than long long
>> > arrays and vect-iv-9.c updated to scan for the
>> > "Vectorized 1 loops" string twice. I've verified by hand by building a
>> > sparc-sun-solaris2.10 cc1 that compiling with
>> > -mcpu=ultrasparc -mvis gives the right strings in the vectoriser dump.
>> > 
>> > So is this version ok?
>> > 
>> 
>> So richi was ok with the implementation part of the patch [1]
>> Are the testuite changes made since ok?
>> 
>> [1] https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00177.html
>> [2] https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00331.html
>
> Ok with me - I wondered if Rainer wanted to double-check.

I just ran a sparc-sun-solaris2.12 bootstrap with the patch(es) applied
and results look good: the remaining failures are unrelated and have
already been reported separately.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University