[Bug target/103316] New: PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[Bug target/103316] New: PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

            Bug ID: 103316
           Summary: PowerPC: Gimple folding of int128 comparisons produces
                    suboptimal code
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wschmidt at gcc dot gnu.org
  Target Milestone: ---

While working on replacing the old builtins infrastructure for the rs6000 back
end, I observed that we generate poor code for 128-bit comparisons if we enable
gimple folding on such builtins.  These included (old internal names):

  P10V_BUILTIN_VCMPEQUT
  P10V_BUILTIN_CMPNET
  P10V_BUILTIN_CMPGE_1TI
  P10V_BUILTIN_CMPGE_U1TI
  P10V_BUILTIN_VCMPGTUT
  P10V_BUILTIN_VCMPGTST
  P10V_BUILTIN_CMPLE_1TI
  P10V_BUILTIN_CMPLE_U1TI

I disabled gimple folding for these with the new support, but we need to look
into why we generated bad code here.  I suspect it's something as simple as
missing optab entries.

This isn't a regression, just a potential improvement.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|UNCONFIRMED                 |NEW
                 CC|                            |bergner at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org
     Ever confirmed|0                           |1
             Target|                            |powerpc*-*-*
   Last reconfirmed|                            |2021-11-18
   Target Milestone|---                         |12.0

--- Comment #1 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Confirmed.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.0                        |---

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #2 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Do you maybe have some simple example (of what we generate, and what you say
it should be)?

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #3 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Sure.  Consider:

#include <altivec.h>

vector bool __int128
foo (vector signed __int128 a, vector signed __int128 b)
{
  return vec_cmpgt (a, b);
}

With gimple folding we emulate in 64-bit mode:

        mfvsrd 9,34
        mfvsrld 8,34
        mfvsrd 11,35
        mfvsrld 10,35
        li 7,1
        cmpd 0,9,11
        bgt 0,.L2
        cmpld 0,9,11
        beq 0,.L5
.L3:
        li 7,0
.L2:
        subfic 10,7,0
        subfe 11,11,11
        mtvsrdd 34,11,10
        blr
        .p2align 4,,15
.L5:
        cmpld 0,8,10
        bgt 0,.L2
        b .L3

The desired code generation is just

        vcmpgtsq 2,2,3

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #4 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Above was compiled with -O2 -mcpu=power10.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #5 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
At first glance, this is probably because vector.md's definition of
vec_cmp<mode><mode> isn't defined for V1TImode.  Probably needs to be changed
to use VEC_IP rather than VEC_I and implement all the cases for 128-bit.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #6 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Ah, now I see.  Thanks!

Power10 has some new 128-bit insns (and p9 and p8 did before, too).

I still think it would be best if Gimple did *never* split data.  It
simply does not know enough about the machine and what the eventual
machine code will be like to do so advantageously.  This is the kind
of thing that RTL can do much better, is much better positioned to do
(and in fact it does do this, in all subregN passes).

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #6)
> Ah, now I see.  Thanks!
> 
> Power10 has some new 128-bit insns (and p9 and p8 did before, too).
> 
> I still think it would be best if Gimple did *never* split data.  It
> simply does not know enough about the machine and what the eventual
> machine code will be like to do so advantageously.  This is the kind
> of thing that RTL can do much better, is much better positioned to do
> (and in fact it does do this, in all subregN passes).

Well, we need to be able to RTL expand the GIMPLE and vector lowering
will ensure we can.  Otherwise we'll just ICE ;)

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Btw:

> 	mfvsrd 9,34
> 	mfvsrld 8,34
> 	mfvsrd 11,35
> 	mfvsrld 10,35
> 	li 7,1
> 	cmpd 0,9,11
> 	bgt 0,.L2
> 	cmpld 0,9,11
> 	beq 0,.L5
> .L3:
>     	li 7,0

The fall-through here makes the code worse.

> .L2:
>     	subfic 10,7,0
> 	subfe 11,11,11

And we shouldn't generate this for p10 at all anyway!  Something
with setbc would be better.

If there were no branches here RTL could have made the code a bit
more reasonable again, but with branches, no such chance :-(

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #9 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> > I still think it would be best if Gimple did *never* split data.  It
> > simply does not know enough about the machine and what the eventual
> > machine code will be like to do so advantageously.  This is the kind
> > of thing that RTL can do much better, is much better positioned to do
> > (and in fact it does do this, in all subregN passes).
> 
> Well, we need to be able to RTL expand the GIMPLE and vector lowering
> will ensure we can.  Otherwise we'll just ICE ;)

Aha.  But RTL can handle this itself already.  There is just a pass ordering
problem maybe?

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #10 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
FWIW, I think the vector lowering pass is reasonable.  These things always look
horrible in isolation, but the lowering allows more optimization when the
target doesn't really support the data type.

This is just an oversight in our back end, and once we correct it, this should
all fall out nicely (I hope).

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #11 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
As I mentioned privately, we could do with an audit of our implementation of
standard patterns in general, since  we tend to find such missing cases more
often than I'd like...

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #12 from Segher Boessenkool <segher at gcc dot gnu.org> ---
When is the lowering done currently?  Only for the ops that have no other way
of doing, and things are merged back to an __int128 immediately after that?

If that is what is going on, then that is unfortunate, but it may the best
we can do yes.  Sigh.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #13 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Bill Schmidt from comment #11)
> As I mentioned privately, we could do with an audit of our implementation of
> standard patterns in general, since  we tend to find such missing cases more
> often than I'd like...

It would be great if there was some standard way of seeing what targets
implement (and do not implement!) what.  It will be hugely useful when
implementing a new port for example, but also great when maintaining one.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #14 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #13)
> (In reply to Bill Schmidt from comment #11)
> > As I mentioned privately, we could do with an audit of our implementation of
> > standard patterns in general, since  we tend to find such missing cases more
> > often than I'd like...
> 
> It would be great if there was some standard way of seeing what targets
> implement (and do not implement!) what.  It will be hugely useful when
> implementing a new port for example, but also great when maintaining one.

I agree.  Some tooling around this would be a big benefit to the community. 
Maybe some GSOC person would like to do that. :)

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[guihaoc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

HaoChen Gui <guihaoc at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |guihaoc at gcc dot gnu.org

--- Comment #15 from HaoChen Gui <guihaoc at gcc dot gnu.org> ---
I drafted a patch to define separate expanders for V1TI. It works. I wonder if
I shall add V1TI into VEC_I and define an unified expander for V16QI V8HI V4SI
V2DI and V1TI. Also some insn patterns should be merged then. Please advice.
Thanks a lot.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #16 from Segher Boessenkool <segher at gcc dot gnu.org> ---
There are many patterns that use VEC_I, and not all have a V1TI variant
currently, so adding V1TI to it is not suitable for now.  It is better to
add a new iterator for now.

This whole thing desperately needs a big cleanup :-(

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by HaoChen Gui <guihaoc@gcc.gnu.org>:

https://gcc.gnu.org/g:8d1c6e7038b0c281ac2678f2f615806a7aac9174

commit r13-1131-g8d1c6e7038b0c281ac2678f2f615806a7aac9174
Author: Haochen Gui <guihaoc@gcc.gnu.org>
Date:   Mon May 30 09:12:34 2022 +0800

    rs6000: add V1TI into vector comparison expand [PR103316]

    This patch adds V1TI mode into a new mode iterator used in vector
comparison,shift and rotation expands.  It also merges some vector comparison,
shift and rotation expands for V1T1 and other vector integer modes as they have
the similar patterns.  The expands for V1TI only are removed.

    gcc/
            PR target/103316
            * config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_builtin):
Enable
            gimple folding for RS6000_BIF_VCMPEQUT, RS6000_BIF_VCMPNET,
            RS6000_BIF_CMPGE_1TI, RS6000_BIF_CMPGE_U1TI, RS6000_BIF_VCMPGTUT,
            RS6000_BIF_VCMPGTST, RS6000_BIF_CMPLE_1TI, RS6000_BIF_CMPLE_U1TI.
            * config/rs6000/vector.md (VEC_IC): New mode iterator.  Add support
            for new Power10 V1TI instructions.
            (vec_cmp<mode><mode>): Set mode iterator to VEC_IC.
            (vec_cmpu<mode><mode>): Likewise.
            (vector_nlt<mode>): Set mode iterator to VEC_IC.
            (vector_nltv1ti): Remove.
            (vector_gtu<mode>): Set mode iterator to VEC_IC.
            (vector_gtuv1ti): Remove.
            (vector_nltu<mode>): Set mode iterator to VEC_IC.
            (vector_nltuv1ti): Remove.
            (vector_geu<mode>): Set mode iterator to VEC_IC.
            (vector_ngt<mode>): Likewise.
            (vector_ngtv1ti): Remove.
            (vector_ngtu<mode>): Set mode iterator to VEC_IC.
            (vector_ngtuv1ti): Remove.
            (vector_gtu_<mode>_p): Set mode iterator to VEC_IC.
            (vector_gtu_v1ti_p): Remove.
            (vrotl<mode>3): Set mode iterator to VEC_IC.  Emit insns for V1TI.
            (vrotlv1ti3): Remove.
            (vashr<mode>3): Set mode iterator to VEC_IC.  Emit insns for V1TI.
            (vashrv1ti3): Remove.

    gcc/testsuite/
            PR target/103316
            * gcc.target/powerpc/pr103316.c: New.
            * gcc.target/powerpc/fold-vec-cmp-int128.c: New.

[Bug target/103316] PowerPC: Gimple folding of int128 comparisons produces suboptimal code

[guihaoc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103316

HaoChen Gui <guihaoc at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
           Assignee|unassigned at gcc dot gnu.org      |guihaoc at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #18 from HaoChen Gui <guihaoc at gcc dot gnu.org> ---
Fixed by r13-1131