[v2,0/9] S/390: Use signaling FP comparison instructions
mbox series

Message ID 20190822134551.18924-1-iii@linux.ibm.com
Headers show
Series
  • S/390: Use signaling FP comparison instructions
Related show

Message

Ilya Leoshkevich Aug. 22, 2019, 1:45 p.m. UTC
Bootstrap and regtest running on x86_64-redhat-linux and
s390x-redhat-linux.

This patch series adds signaling FP comparison support (both scalar and
vector) to s390 backend.

Patch 1 documents the current behavior of MIN, MAX and LTGT operations
with respect to signaling.

Patches 2-4 make it possible to query supported vcond rtxes and make
use of that for z13.

Patches 5-7 are preparation cleanups.

Patch 8 is an actual implementation.

Path 9 contains new tests, that make sure autovectorized comparisons use
proper instructions.

Ilya Leoshkevich (9):
  Document signaling for min, max and ltgt operations
  hash_traits: split pointer_hash_mark from pointer_hash
  Introduce can_vector_compare_p function
  S/390: Do not use signaling vector comparisons on z13
  S/390: Implement vcond expander for V1TI,V1TF
  S/390: Remove code duplication in vec_unordered<mode>
  S/390: Remove code duplication in vec_* comparison expanders
  S/390: Use signaling FP comparison instructions
  S/390: Test signaling FP comparison instructions

v1->v2:
Improve wording in documentation commit message.
Replace hook with optabs query.
Add signaling eq test.

 gcc/Makefile.in                               |   2 +-
 gcc/config/s390/2827.md                       |  14 +-
 gcc/config/s390/2964.md                       |  13 +-
 gcc/config/s390/3906.md                       |  17 +-
 gcc/config/s390/8561.md                       |  19 +-
 gcc/config/s390/s390-builtins.def             |  16 +-
 gcc/config/s390/s390-modes.def                |   8 +
 gcc/config/s390/s390.c                        |  38 ++-
 gcc/config/s390/s390.md                       |  14 +
 gcc/config/s390/vector.md                     | 283 ++++++++++++------
 gcc/cp/decl2.c                                |  14 +-
 gcc/doc/generic.texi                          |  16 +-
 gcc/doc/md.texi                               |   3 +-
 gcc/doc/rtl.texi                              |   3 +-
 gcc/hash-traits.h                             |  74 +++--
 gcc/ipa-prop.c                                |  47 +--
 gcc/optabs-tree.c                             |  11 +-
 gcc/optabs.c                                  |  79 +++++
 gcc/optabs.h                                  |  15 +
 gcc/testsuite/gcc.target/s390/s390.exp        |   8 +
 .../gcc.target/s390/vector/vec-scalar-cmp-1.c |   8 +-
 .../s390/zvector/autovec-double-quiet-eq.c    |   8 +
 .../s390/zvector/autovec-double-quiet-ge.c    |   8 +
 .../s390/zvector/autovec-double-quiet-gt.c    |   8 +
 .../s390/zvector/autovec-double-quiet-le.c    |   8 +
 .../s390/zvector/autovec-double-quiet-lt.c    |   8 +
 .../zvector/autovec-double-quiet-ordered.c    |  10 +
 .../s390/zvector/autovec-double-quiet-uneq.c  |  10 +
 .../zvector/autovec-double-quiet-unordered.c  |  11 +
 .../autovec-double-signaling-eq-z13-finite.c  |  10 +
 .../zvector/autovec-double-signaling-eq-z13.c |   9 +
 .../zvector/autovec-double-signaling-eq.c     |  11 +
 .../autovec-double-signaling-ge-z13-finite.c  |  10 +
 .../zvector/autovec-double-signaling-ge-z13.c |   9 +
 .../zvector/autovec-double-signaling-ge.c     |   8 +
 .../autovec-double-signaling-gt-z13-finite.c  |  10 +
 .../zvector/autovec-double-signaling-gt-z13.c |   9 +
 .../zvector/autovec-double-signaling-gt.c     |   8 +
 .../autovec-double-signaling-le-z13-finite.c  |  10 +
 .../zvector/autovec-double-signaling-le-z13.c |   9 +
 .../zvector/autovec-double-signaling-le.c     |   8 +
 .../autovec-double-signaling-lt-z13-finite.c  |  10 +
 .../zvector/autovec-double-signaling-lt-z13.c |   9 +
 .../zvector/autovec-double-signaling-lt.c     |   8 +
 ...autovec-double-signaling-ltgt-z13-finite.c |   9 +
 .../autovec-double-signaling-ltgt-z13.c       |   9 +
 .../zvector/autovec-double-signaling-ltgt.c   |   9 +
 .../s390/zvector/autovec-double-smax-z13.F90  |  11 +
 .../s390/zvector/autovec-double-smax.F90      |   8 +
 .../s390/zvector/autovec-double-smin-z13.F90  |  11 +
 .../s390/zvector/autovec-double-smin.F90      |   8 +
 .../s390/zvector/autovec-float-quiet-eq.c     |   8 +
 .../s390/zvector/autovec-float-quiet-ge.c     |   8 +
 .../s390/zvector/autovec-float-quiet-gt.c     |   8 +
 .../s390/zvector/autovec-float-quiet-le.c     |   8 +
 .../s390/zvector/autovec-float-quiet-lt.c     |   8 +
 .../zvector/autovec-float-quiet-ordered.c     |  10 +
 .../s390/zvector/autovec-float-quiet-uneq.c   |  10 +
 .../zvector/autovec-float-quiet-unordered.c   |  11 +
 .../s390/zvector/autovec-float-signaling-eq.c |  11 +
 .../s390/zvector/autovec-float-signaling-ge.c |   8 +
 .../s390/zvector/autovec-float-signaling-gt.c |   8 +
 .../s390/zvector/autovec-float-signaling-le.c |   8 +
 .../s390/zvector/autovec-float-signaling-lt.c |   8 +
 .../zvector/autovec-float-signaling-ltgt.c    |   9 +
 .../gcc.target/s390/zvector/autovec-fortran.h |   7 +
 .../autovec-long-double-signaling-ge.c        |   8 +
 .../autovec-long-double-signaling-gt.c        |   8 +
 .../autovec-long-double-signaling-le.c        |   8 +
 .../autovec-long-double-signaling-lt.c        |   8 +
 .../gcc.target/s390/zvector/autovec.h         |  41 +++
 71 files changed, 946 insertions(+), 233 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-eq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-ge.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-gt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-le.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-lt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-ordered.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-uneq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-quiet-unordered.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-eq-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-eq-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-eq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ge-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ge-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ge.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-gt-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-gt-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-gt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-le-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-le-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-le.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-lt-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-lt-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-lt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ltgt-z13-finite.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ltgt-z13.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-signaling-ltgt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-smax-z13.F90
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-smax.F90
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-smin-z13.F90
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-double-smin.F90
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-eq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-ge.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-gt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-le.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-lt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-ordered.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-uneq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-quiet-unordered.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-eq.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-ge.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-gt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-le.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-lt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-float-signaling-ltgt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-fortran.h
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-long-double-signaling-ge.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-long-double-signaling-gt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-long-double-signaling-le.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec-long-double-signaling-lt.c
 create mode 100644 gcc/testsuite/gcc.target/s390/zvector/autovec.h

Comments

Ilya Leoshkevich Aug. 29, 2019, 3:39 p.m. UTC | #1
> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> 
> Bootstrap and regtest running on x86_64-redhat-linux and
> s390x-redhat-linux.
> 
> This patch series adds signaling FP comparison support (both scalar and
> vector) to s390 backend.

I'm running into a problem on ppc64 with this patch, and it would be
great if someone could help me figure out the best way to resolve it.

vector36.C test is failing because gimplifier produces the following

  _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
  _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;

from

  VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
                  { -1, -1, -1, -1 } ,
                  { 0, 0, 0, 0 } >

Since the comparison tree code is now hidden behind a temporary, my code
does not have anything to pass to the backend.  The reason for creating
a temporary is that the comparison can trap, and so the following check
in gimplify_expr fails:

  if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
    goto out;

gimple_test_f is is_gimple_condexpr, and it eventually calls
operation_could_trap_p (GT).

My current solution is to simply state that backend does not support
SSA_NAME in vector comparisons, however, I don't like it, since it may
cause performance regressions due to having to fall back to scalar
comparisons.

I was thinking about two other possible solutions:

1. Change the gimplifier to allow trapping vector comparisons.  That's
   a bit complicated, because tree_could_throw_p checks not only for
   floating point traps, but also e.g. for array index out of bounds
   traps.  So I would have to create a tree_could_throw_p version which
   disregards specific kinds of traps.

2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
   its tree_code instead of SSA_NAME.  The potential problem I see with
   this is that there appears to be no guarantee that _5 will be inlined
   into _6 at a later point.  So if we say that we don't need to fall
   back to scalar comparisons based on availability of vector >
   instruction and inlining does not happen, then what's actually will
   be required is vector selection (vsel on S/390), which might not be
   available in general case.

What would be a better way to proceed here?
Richard Biener Aug. 30, 2019, 7:12 a.m. UTC | #2
On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >
> > Bootstrap and regtest running on x86_64-redhat-linux and
> > s390x-redhat-linux.
> >
> > This patch series adds signaling FP comparison support (both scalar and
> > vector) to s390 backend.
>
> I'm running into a problem on ppc64 with this patch, and it would be
> great if someone could help me figure out the best way to resolve it.
>
> vector36.C test is failing because gimplifier produces the following
>
>   _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>   _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>
> from
>
>   VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>                   { -1, -1, -1, -1 } ,
>                   { 0, 0, 0, 0 } >
>
> Since the comparison tree code is now hidden behind a temporary, my code
> does not have anything to pass to the backend.  The reason for creating
> a temporary is that the comparison can trap, and so the following check
> in gimplify_expr fails:
>
>   if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>     goto out;
>
> gimple_test_f is is_gimple_condexpr, and it eventually calls
> operation_could_trap_p (GT).
>
> My current solution is to simply state that backend does not support
> SSA_NAME in vector comparisons, however, I don't like it, since it may
> cause performance regressions due to having to fall back to scalar
> comparisons.
>
> I was thinking about two other possible solutions:
>
> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>    a bit complicated, because tree_could_throw_p checks not only for
>    floating point traps, but also e.g. for array index out of bounds
>    traps.  So I would have to create a tree_could_throw_p version which
>    disregards specific kinds of traps.
>
> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>    its tree_code instead of SSA_NAME.  The potential problem I see with
>    this is that there appears to be no guarantee that _5 will be inlined
>    into _6 at a later point.  So if we say that we don't need to fall
>    back to scalar comparisons based on availability of vector >
>    instruction and inlining does not happen, then what's actually will
>    be required is vector selection (vsel on S/390), which might not be
>    available in general case.
>
> What would be a better way to proceed here?

On GIMPLE there isn't a good reason to split out trapping comparisons
from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
where it is important because we'd have no way to represent EH info
when not done.  It might be a bit awkward to preserve EH across RTL
expansion though in case the [VEC_]COND_EXPR are not expanded
as a single pattern, but I'm not sure.

To go this route you'd have to split the is_gimple_condexpr check
I guess and eventually users turning [VEC_]COND_EXPR into conditional
code (do we have any?) have to be extra careful then.

Richard.

>
Richard Biener Aug. 30, 2019, 7:16 a.m. UTC | #3
On Fri, Aug 30, 2019 at 9:12 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >
> > > Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> > >
> > > Bootstrap and regtest running on x86_64-redhat-linux and
> > > s390x-redhat-linux.
> > >
> > > This patch series adds signaling FP comparison support (both scalar and
> > > vector) to s390 backend.
> >
> > I'm running into a problem on ppc64 with this patch, and it would be
> > great if someone could help me figure out the best way to resolve it.
> >
> > vector36.C test is failing because gimplifier produces the following
> >
> >   _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
> >   _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
> >
> > from
> >
> >   VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
> >                   { -1, -1, -1, -1 } ,
> >                   { 0, 0, 0, 0 } >
> >
> > Since the comparison tree code is now hidden behind a temporary, my code
> > does not have anything to pass to the backend.  The reason for creating
> > a temporary is that the comparison can trap, and so the following check
> > in gimplify_expr fails:
> >
> >   if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
> >     goto out;
> >
> > gimple_test_f is is_gimple_condexpr, and it eventually calls
> > operation_could_trap_p (GT).
> >
> > My current solution is to simply state that backend does not support
> > SSA_NAME in vector comparisons, however, I don't like it, since it may
> > cause performance regressions due to having to fall back to scalar
> > comparisons.
> >
> > I was thinking about two other possible solutions:
> >
> > 1. Change the gimplifier to allow trapping vector comparisons.  That's
> >    a bit complicated, because tree_could_throw_p checks not only for
> >    floating point traps, but also e.g. for array index out of bounds
> >    traps.  So I would have to create a tree_could_throw_p version which
> >    disregards specific kinds of traps.
> >
> > 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
> >    its tree_code instead of SSA_NAME.  The potential problem I see with
> >    this is that there appears to be no guarantee that _5 will be inlined
> >    into _6 at a later point.  So if we say that we don't need to fall
> >    back to scalar comparisons based on availability of vector >
> >    instruction and inlining does not happen, then what's actually will
> >    be required is vector selection (vsel on S/390), which might not be
> >    available in general case.
> >
> > What would be a better way to proceed here?
>
> On GIMPLE there isn't a good reason to split out trapping comparisons
> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> where it is important because we'd have no way to represent EH info
> when not done.  It might be a bit awkward to preserve EH across RTL
> expansion though in case the [VEC_]COND_EXPR are not expanded
> as a single pattern, but I'm not sure.
>
> To go this route you'd have to split the is_gimple_condexpr check
> I guess and eventually users turning [VEC_]COND_EXPR into conditional
> code (do we have any?) have to be extra careful then.

Oh, btw - the fact that we have an expression embedded in [VEC_]COND_EXPR
is something that bothers me for quite some time already and it makes
things like VN awkward and GIMPLE fincky.  We've discussed alternatives
to dead with the simplest being moving the comparison out to a separate
stmt and others like having four operand [VEC_]COND_{EQ,NE,...}_EXPR
codes or simply treating {EQ,NE,...}_EXPR as quarternary on GIMPLE
with either optional 3rd and 4th operand (defaulting to boolean_true/false_node)
or always explicit ones (and thus dropping [VEC_]COND_EXPR).

What does LLVM do here?

Richard.

> Richard.
>
> >
Segher Boessenkool Aug. 30, 2019, 10:06 a.m. UTC | #4
On Fri, Aug 30, 2019 at 09:12:11AM +0200, Richard Biener wrote:
> On GIMPLE there isn't a good reason to split out trapping comparisons
> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> where it is important because we'd have no way to represent EH info
> when not done.  It might be a bit awkward to preserve EH across RTL
> expansion though in case the [VEC_]COND_EXPR are not expanded
> as a single pattern, but I'm not sure.

The *language* specifies which comparisons trap on unordered and which
do not.  (Hopefully it is similar for all or this is going to be a huge
mess :-) )  So Gimple needs to at least keep track of this.  There also
are various optimisations that can be done -- two signaling comparisons
with the same arguments can be folded to just one, for example -- so it
seems to me you want to represent this in Gimple, never mind if you do
EH for them or just a magical trap or whatever.


Segher
Richard Biener Aug. 30, 2019, 10:36 a.m. UTC | #5
On Fri, Aug 30, 2019 at 12:06 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Aug 30, 2019 at 09:12:11AM +0200, Richard Biener wrote:
> > On GIMPLE there isn't a good reason to split out trapping comparisons
> > from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> > where it is important because we'd have no way to represent EH info
> > when not done.  It might be a bit awkward to preserve EH across RTL
> > expansion though in case the [VEC_]COND_EXPR are not expanded
> > as a single pattern, but I'm not sure.
>
> The *language* specifies which comparisons trap on unordered and which
> do not.  (Hopefully it is similar for all or this is going to be a huge
> mess :-) )  So Gimple needs to at least keep track of this.  There also
> are various optimisations that can be done -- two signaling comparisons
> with the same arguments can be folded to just one, for example -- so it
> seems to me you want to represent this in Gimple, never mind if you do
> EH for them or just a magical trap or whatever.

The issue with GIMPLE_CONDs is that we can't have EH edges out
of blocks ending in them so to represent the compare-and-jump
single GIMPLE insn raising exceptions we need to split the actually
trapping compare away.

For [VEC_]COND_EXPR we'd need to say the whole expression
can throw/trap if the embedded comparison can.  On GIMPLE this
is enough precision but we of course have to handle it that way
then.  Currently operation_could_trap* do not consider [VEC_]COND_EXPR
trapping nor does tree_could_trap_p.  stmt_could_throw_1_p looks
fine by means of falling back to checking individual operands and
then running into the embedded comparison.  But we do have
operation_could_trap* callers which would need to be adjusted
on the GIMPLE side.

Richard.

>
> Segher
Ilya Leoshkevich Aug. 30, 2019, 2:19 p.m. UTC | #6
> Am 30.08.2019 um 09:16 schrieb Richard Biener <richard.guenther@gmail.com>:
> 
> On Fri, Aug 30, 2019 at 9:12 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
>> 
>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>> 
>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>>> 
>>>> Bootstrap and regtest running on x86_64-redhat-linux and
>>>> s390x-redhat-linux.
>>>> 
>>>> This patch series adds signaling FP comparison support (both scalar and
>>>> vector) to s390 backend.
>>> 
>>> I'm running into a problem on ppc64 with this patch, and it would be
>>> great if someone could help me figure out the best way to resolve it.
>>> 
>>> vector36.C test is failing because gimplifier produces the following
>>> 
>>>  _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>>>  _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>>> 
>>> from
>>> 
>>>  VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>>>                  { -1, -1, -1, -1 } ,
>>>                  { 0, 0, 0, 0 } >
>>> 
>>> Since the comparison tree code is now hidden behind a temporary, my code
>>> does not have anything to pass to the backend.  The reason for creating
>>> a temporary is that the comparison can trap, and so the following check
>>> in gimplify_expr fails:
>>> 
>>>  if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>>>    goto out;
>>> 
>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
>>> operation_could_trap_p (GT).
>>> 
>>> My current solution is to simply state that backend does not support
>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
>>> cause performance regressions due to having to fall back to scalar
>>> comparisons.
>>> 
>>> I was thinking about two other possible solutions:
>>> 
>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>>>   a bit complicated, because tree_could_throw_p checks not only for
>>>   floating point traps, but also e.g. for array index out of bounds
>>>   traps.  So I would have to create a tree_could_throw_p version which
>>>   disregards specific kinds of traps.
>>> 
>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>>>   its tree_code instead of SSA_NAME.  The potential problem I see with
>>>   this is that there appears to be no guarantee that _5 will be inlined
>>>   into _6 at a later point.  So if we say that we don't need to fall
>>>   back to scalar comparisons based on availability of vector >
>>>   instruction and inlining does not happen, then what's actually will
>>>   be required is vector selection (vsel on S/390), which might not be
>>>   available in general case.
>>> 
>>> What would be a better way to proceed here?
>> 
>> On GIMPLE there isn't a good reason to split out trapping comparisons
>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
>> where it is important because we'd have no way to represent EH info
>> when not done.  It might be a bit awkward to preserve EH across RTL
>> expansion though in case the [VEC_]COND_EXPR are not expanded
>> as a single pattern, but I'm not sure.
>> 
>> To go this route you'd have to split the is_gimple_condexpr check
>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
>> code (do we have any?) have to be extra careful then.
> 
> Oh, btw - the fact that we have an expression embedded in [VEC_]COND_EXPR
> is something that bothers me for quite some time already and it makes
> things like VN awkward and GIMPLE fincky.  We've discussed alternatives
> to dead with the simplest being moving the comparison out to a separate
> stmt and others like having four operand [VEC_]COND_{EQ,NE,...}_EXPR
> codes or simply treating {EQ,NE,...}_EXPR as quarternary on GIMPLE
> with either optional 3rd and 4th operand (defaulting to boolean_true/false_node)
> or always explicit ones (and thus dropping [VEC_]COND_EXPR).
> 
> What does LLVM do here?

For

void f(long long * restrict w, double * restrict x, double * restrict y, int n)
{
        for (int i = 0; i < n; i++)
                w[i] = x[i] == y[i] ? x[i] : y[i];
}

LLVM does

  %26 = fcmp oeq <2 x double> %21, %25
  %27 = extractelement <2 x i1> %26, i32 0
  %28 = select <2 x i1> %26, <2 x double> %21, <2 x double> %25

So they have separate operations for comparisons and ternary operator
(fcmp + select).
Ilya Leoshkevich Aug. 30, 2019, 2:40 p.m. UTC | #7
> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
> 
> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>> 
>>> Bootstrap and regtest running on x86_64-redhat-linux and
>>> s390x-redhat-linux.
>>> 
>>> This patch series adds signaling FP comparison support (both scalar and
>>> vector) to s390 backend.
>> 
>> I'm running into a problem on ppc64 with this patch, and it would be
>> great if someone could help me figure out the best way to resolve it.
>> 
>> vector36.C test is failing because gimplifier produces the following
>> 
>>  _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>>  _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>> 
>> from
>> 
>>  VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>>                  { -1, -1, -1, -1 } ,
>>                  { 0, 0, 0, 0 } >
>> 
>> Since the comparison tree code is now hidden behind a temporary, my code
>> does not have anything to pass to the backend.  The reason for creating
>> a temporary is that the comparison can trap, and so the following check
>> in gimplify_expr fails:
>> 
>>  if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>>    goto out;
>> 
>> gimple_test_f is is_gimple_condexpr, and it eventually calls
>> operation_could_trap_p (GT).
>> 
>> My current solution is to simply state that backend does not support
>> SSA_NAME in vector comparisons, however, I don't like it, since it may
>> cause performance regressions due to having to fall back to scalar
>> comparisons.
>> 
>> I was thinking about two other possible solutions:
>> 
>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>>   a bit complicated, because tree_could_throw_p checks not only for
>>   floating point traps, but also e.g. for array index out of bounds
>>   traps.  So I would have to create a tree_could_throw_p version which
>>   disregards specific kinds of traps.
>> 
>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>>   its tree_code instead of SSA_NAME.  The potential problem I see with
>>   this is that there appears to be no guarantee that _5 will be inlined
>>   into _6 at a later point.  So if we say that we don't need to fall
>>   back to scalar comparisons based on availability of vector >
>>   instruction and inlining does not happen, then what's actually will
>>   be required is vector selection (vsel on S/390), which might not be
>>   available in general case.
>> 
>> What would be a better way to proceed here?
> 
> On GIMPLE there isn't a good reason to split out trapping comparisons
> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> where it is important because we'd have no way to represent EH info
> when not done.  It might be a bit awkward to preserve EH across RTL
> expansion though in case the [VEC_]COND_EXPR are not expanded
> as a single pattern, but I'm not sure.

Ok, so I'm testing the following now - for the problematic test that
helped:

diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
index b0c9f9b671a..940aa394769 100644
--- a/gcc/gimple-expr.c
+++ b/gcc/gimple-expr.c
@@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
 	  || TREE_CODE (t) == BIT_FIELD_REF);
 }
 
-/*  Return true if T is a GIMPLE condition.  */
+/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
 
-bool
-is_gimple_condexpr (tree t)
+static bool
+is_gimple_condexpr_1 (tree t, bool allow_traps)
 {
   return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
-				&& !tree_could_throw_p (t)
+				&& (allow_traps || !tree_could_throw_p (t))
 				&& is_gimple_val (TREE_OPERAND (t, 0))
 				&& is_gimple_val (TREE_OPERAND (t, 1))));
 }
 
+/*  Return true if T is a GIMPLE condition.  */
+
+bool
+is_gimple_condexpr (tree t)
+{
+  return is_gimple_condexpr_1 (t, false);
+}
+
+/* Like is_gimple_condexpr, but allow the T to trap.  */
+
+bool
+is_possibly_trapping_gimple_condexpr (tree t)
+{
+  return is_gimple_condexpr_1 (t, true);
+}
+
 /* Return true if T is a gimple address.  */
 
 bool
diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
index 1ad1432bd17..20546ca5b99 100644
--- a/gcc/gimple-expr.h
+++ b/gcc/gimple-expr.h
@@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
 					   tree *);
 extern bool is_gimple_lvalue (tree);
 extern bool is_gimple_condexpr (tree);
+extern bool is_possibly_trapping_gimple_condexpr (tree);
 extern bool is_gimple_address (const_tree);
 extern bool is_gimple_invariant_address (const_tree);
 extern bool is_gimple_ip_invariant_address (const_tree);
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index daa0b71c191..4e6256390c0 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   else if (gimple_test_f == is_gimple_val
            || gimple_test_f == is_gimple_call_addr
            || gimple_test_f == is_gimple_condexpr
+	   || gimple_test_f == is_possibly_trapping_gimple_condexpr
            || gimple_test_f == is_gimple_mem_rhs
            || gimple_test_f == is_gimple_mem_rhs_or_call
            || gimple_test_f == is_gimple_reg_rhs
@@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	    enum gimplify_status r0, r1, r2;
 
 	    r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
-				post_p, is_gimple_condexpr, fb_rvalue);
+				post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
 	    r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
 				post_p, is_gimple_val, fb_rvalue);
 	    r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index b75fdb2e63f..175b858f56b 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
       return true;
     }
 
-  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
-       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
+  if ((rhs_code == VEC_COND_EXPR
+       ? !is_possibly_trapping_gimple_condexpr (rhs1)
+       : (rhs_code == COND_EXPR
+	  ? !is_gimple_condexpr (rhs1)
+	  : !is_gimple_val (rhs1)))
       || !is_gimple_val (rhs2)
       || !is_gimple_val (rhs3))
     {

> 
> To go this route you'd have to split the is_gimple_condexpr check
> I guess and eventually users turning [VEC_]COND_EXPR into conditional
> code (do we have any?) have to be extra careful then.
> 

We have expand_vector_condition, which turns VEC_COND_EXPR into
COND_EXPR - but this should be harmless, right?  I could not find
anything else.
Ilya Leoshkevich Aug. 30, 2019, 3:24 p.m. UTC | #8
> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> 
>> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
>> 
>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>> 
>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>>> 
>>>> Bootstrap and regtest running on x86_64-redhat-linux and
>>>> s390x-redhat-linux.
>>>> 
>>>> This patch series adds signaling FP comparison support (both scalar and
>>>> vector) to s390 backend.
>>> 
>>> I'm running into a problem on ppc64 with this patch, and it would be
>>> great if someone could help me figure out the best way to resolve it.
>>> 
>>> vector36.C test is failing because gimplifier produces the following
>>> 
>>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>>> 
>>> from
>>> 
>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>>>                 { -1, -1, -1, -1 } ,
>>>                 { 0, 0, 0, 0 } >
>>> 
>>> Since the comparison tree code is now hidden behind a temporary, my code
>>> does not have anything to pass to the backend.  The reason for creating
>>> a temporary is that the comparison can trap, and so the following check
>>> in gimplify_expr fails:
>>> 
>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>>>   goto out;
>>> 
>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
>>> operation_could_trap_p (GT).
>>> 
>>> My current solution is to simply state that backend does not support
>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
>>> cause performance regressions due to having to fall back to scalar
>>> comparisons.
>>> 
>>> I was thinking about two other possible solutions:
>>> 
>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>>>  a bit complicated, because tree_could_throw_p checks not only for
>>>  floating point traps, but also e.g. for array index out of bounds
>>>  traps.  So I would have to create a tree_could_throw_p version which
>>>  disregards specific kinds of traps.
>>> 
>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>>>  its tree_code instead of SSA_NAME.  The potential problem I see with
>>>  this is that there appears to be no guarantee that _5 will be inlined
>>>  into _6 at a later point.  So if we say that we don't need to fall
>>>  back to scalar comparisons based on availability of vector >
>>>  instruction and inlining does not happen, then what's actually will
>>>  be required is vector selection (vsel on S/390), which might not be
>>>  available in general case.
>>> 
>>> What would be a better way to proceed here?
>> 
>> On GIMPLE there isn't a good reason to split out trapping comparisons
>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
>> where it is important because we'd have no way to represent EH info
>> when not done.  It might be a bit awkward to preserve EH across RTL
>> expansion though in case the [VEC_]COND_EXPR are not expanded
>> as a single pattern, but I'm not sure.
> 
> Ok, so I'm testing the following now - for the problematic test that
> helped:
> 
> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> index b0c9f9b671a..940aa394769 100644
> --- a/gcc/gimple-expr.c
> +++ b/gcc/gimple-expr.c
> @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
> 	  || TREE_CODE (t) == BIT_FIELD_REF);
> }
> 
> -/*  Return true if T is a GIMPLE condition.  */
> +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
> 
> -bool
> -is_gimple_condexpr (tree t)
> +static bool
> +is_gimple_condexpr_1 (tree t, bool allow_traps)
> {
>   return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
> -				&& !tree_could_throw_p (t)
> +				&& (allow_traps || !tree_could_throw_p (t))
> 				&& is_gimple_val (TREE_OPERAND (t, 0))
> 				&& is_gimple_val (TREE_OPERAND (t, 1))));
> }
> 
> +/*  Return true if T is a GIMPLE condition.  */
> +
> +bool
> +is_gimple_condexpr (tree t)
> +{
> +  return is_gimple_condexpr_1 (t, false);
> +}
> +
> +/* Like is_gimple_condexpr, but allow the T to trap.  */
> +
> +bool
> +is_possibly_trapping_gimple_condexpr (tree t)
> +{
> +  return is_gimple_condexpr_1 (t, true);
> +}
> +
> /* Return true if T is a gimple address.  */
> 
> bool
> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
> index 1ad1432bd17..20546ca5b99 100644
> --- a/gcc/gimple-expr.h
> +++ b/gcc/gimple-expr.h
> @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
> 					   tree *);
> extern bool is_gimple_lvalue (tree);
> extern bool is_gimple_condexpr (tree);
> +extern bool is_possibly_trapping_gimple_condexpr (tree);
> extern bool is_gimple_address (const_tree);
> extern bool is_gimple_invariant_address (const_tree);
> extern bool is_gimple_ip_invariant_address (const_tree);
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index daa0b71c191..4e6256390c0 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>   else if (gimple_test_f == is_gimple_val
>            || gimple_test_f == is_gimple_call_addr
>            || gimple_test_f == is_gimple_condexpr
> +	   || gimple_test_f == is_possibly_trapping_gimple_condexpr
>            || gimple_test_f == is_gimple_mem_rhs
>            || gimple_test_f == is_gimple_mem_rhs_or_call
>            || gimple_test_f == is_gimple_reg_rhs
> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> 	    enum gimplify_status r0, r1, r2;
> 
> 	    r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
> -				post_p, is_gimple_condexpr, fb_rvalue);
> +				post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
> 	    r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
> 				post_p, is_gimple_val, fb_rvalue);
> 	    r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index b75fdb2e63f..175b858f56b 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
>       return true;
>     }
> 
> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
> +  if ((rhs_code == VEC_COND_EXPR
> +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
> +       : (rhs_code == COND_EXPR
> +	  ? !is_gimple_condexpr (rhs1)
> +	  : !is_gimple_val (rhs1)))
>       || !is_gimple_val (rhs2)
>       || !is_gimple_val (rhs3))
>     {
> 
>> 
>> To go this route you'd have to split the is_gimple_condexpr check
>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
>> code (do we have any?) have to be extra careful then.
>> 
> 
> We have expand_vector_condition, which turns VEC_COND_EXPR into
> COND_EXPR - but this should be harmless, right?  I could not find
> anything else.

Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
COND_EXPR usages.  There is, of course, a great deal more code, so I'm
not sure whether I looked exhaustively through it, but there are at
least store_expr and do_jump which do exactly this during expansion.
Should we worry about EH edges at this point?
Richard Biener Sept. 2, 2019, 10:37 a.m. UTC | #9
On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >
> >> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
> >>
> >> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>
> >>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >>>>
> >>>> Bootstrap and regtest running on x86_64-redhat-linux and
> >>>> s390x-redhat-linux.
> >>>>
> >>>> This patch series adds signaling FP comparison support (both scalar and
> >>>> vector) to s390 backend.
> >>>
> >>> I'm running into a problem on ppc64 with this patch, and it would be
> >>> great if someone could help me figure out the best way to resolve it.
> >>>
> >>> vector36.C test is failing because gimplifier produces the following
> >>>
> >>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
> >>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
> >>>
> >>> from
> >>>
> >>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
> >>>                 { -1, -1, -1, -1 } ,
> >>>                 { 0, 0, 0, 0 } >
> >>>
> >>> Since the comparison tree code is now hidden behind a temporary, my code
> >>> does not have anything to pass to the backend.  The reason for creating
> >>> a temporary is that the comparison can trap, and so the following check
> >>> in gimplify_expr fails:
> >>>
> >>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
> >>>   goto out;
> >>>
> >>> gimple_test_f is is_gimple_condexpr, and it eventually calls
> >>> operation_could_trap_p (GT).
> >>>
> >>> My current solution is to simply state that backend does not support
> >>> SSA_NAME in vector comparisons, however, I don't like it, since it may
> >>> cause performance regressions due to having to fall back to scalar
> >>> comparisons.
> >>>
> >>> I was thinking about two other possible solutions:
> >>>
> >>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
> >>>  a bit complicated, because tree_could_throw_p checks not only for
> >>>  floating point traps, but also e.g. for array index out of bounds
> >>>  traps.  So I would have to create a tree_could_throw_p version which
> >>>  disregards specific kinds of traps.
> >>>
> >>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
> >>>  its tree_code instead of SSA_NAME.  The potential problem I see with
> >>>  this is that there appears to be no guarantee that _5 will be inlined
> >>>  into _6 at a later point.  So if we say that we don't need to fall
> >>>  back to scalar comparisons based on availability of vector >
> >>>  instruction and inlining does not happen, then what's actually will
> >>>  be required is vector selection (vsel on S/390), which might not be
> >>>  available in general case.
> >>>
> >>> What would be a better way to proceed here?
> >>
> >> On GIMPLE there isn't a good reason to split out trapping comparisons
> >> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> >> where it is important because we'd have no way to represent EH info
> >> when not done.  It might be a bit awkward to preserve EH across RTL
> >> expansion though in case the [VEC_]COND_EXPR are not expanded
> >> as a single pattern, but I'm not sure.
> >
> > Ok, so I'm testing the following now - for the problematic test that
> > helped:
> >
> > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> > index b0c9f9b671a..940aa394769 100644
> > --- a/gcc/gimple-expr.c
> > +++ b/gcc/gimple-expr.c
> > @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
> >         || TREE_CODE (t) == BIT_FIELD_REF);
> > }
> >
> > -/*  Return true if T is a GIMPLE condition.  */
> > +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
> >
> > -bool
> > -is_gimple_condexpr (tree t)
> > +static bool
> > +is_gimple_condexpr_1 (tree t, bool allow_traps)
> > {
> >   return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
> > -                             && !tree_could_throw_p (t)
> > +                             && (allow_traps || !tree_could_throw_p (t))
> >                               && is_gimple_val (TREE_OPERAND (t, 0))
> >                               && is_gimple_val (TREE_OPERAND (t, 1))));
> > }
> >
> > +/*  Return true if T is a GIMPLE condition.  */
> > +
> > +bool
> > +is_gimple_condexpr (tree t)
> > +{
> > +  return is_gimple_condexpr_1 (t, false);
> > +}
> > +
> > +/* Like is_gimple_condexpr, but allow the T to trap.  */
> > +
> > +bool
> > +is_possibly_trapping_gimple_condexpr (tree t)
> > +{
> > +  return is_gimple_condexpr_1 (t, true);
> > +}
> > +
> > /* Return true if T is a gimple address.  */
> >
> > bool
> > diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
> > index 1ad1432bd17..20546ca5b99 100644
> > --- a/gcc/gimple-expr.h
> > +++ b/gcc/gimple-expr.h
> > @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
> >                                          tree *);
> > extern bool is_gimple_lvalue (tree);
> > extern bool is_gimple_condexpr (tree);
> > +extern bool is_possibly_trapping_gimple_condexpr (tree);
> > extern bool is_gimple_address (const_tree);
> > extern bool is_gimple_invariant_address (const_tree);
> > extern bool is_gimple_ip_invariant_address (const_tree);
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index daa0b71c191..4e6256390c0 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >   else if (gimple_test_f == is_gimple_val
> >            || gimple_test_f == is_gimple_call_addr
> >            || gimple_test_f == is_gimple_condexpr
> > +        || gimple_test_f == is_possibly_trapping_gimple_condexpr
> >            || gimple_test_f == is_gimple_mem_rhs
> >            || gimple_test_f == is_gimple_mem_rhs_or_call
> >            || gimple_test_f == is_gimple_reg_rhs
> > @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >           enum gimplify_status r0, r1, r2;
> >
> >           r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
> > -                             post_p, is_gimple_condexpr, fb_rvalue);
> > +                             post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
> >           r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
> >                               post_p, is_gimple_val, fb_rvalue);
> >           r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index b75fdb2e63f..175b858f56b 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
> >       return true;
> >     }
> >
> > -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
> > -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
> > +  if ((rhs_code == VEC_COND_EXPR
> > +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
> > +       : (rhs_code == COND_EXPR
> > +       ? !is_gimple_condexpr (rhs1)
> > +       : !is_gimple_val (rhs1)))
> >       || !is_gimple_val (rhs2)
> >       || !is_gimple_val (rhs3))
> >     {
> >
> >>
> >> To go this route you'd have to split the is_gimple_condexpr check
> >> I guess and eventually users turning [VEC_]COND_EXPR into conditional
> >> code (do we have any?) have to be extra careful then.
> >>
> >
> > We have expand_vector_condition, which turns VEC_COND_EXPR into
> > COND_EXPR - but this should be harmless, right?  I could not find
> > anything else.
>
> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
> COND_EXPR usages.  There is, of course, a great deal more code, so I'm
> not sure whether I looked exhaustively through it, but there are at
> least store_expr and do_jump which do exactly this during expansion.
> Should we worry about EH edges at this point?

Well, the EH edge needs to persist (and be rooted off the comparison,
not the selection).

That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs
(it allows is_gimple_val which isn't proper form for GIMPLE_COND).  Of course
there's code using it for GIMPLE_CONDs which would need to be adjusted.

Richard.
Ilya Leoshkevich Sept. 2, 2019, 4:28 p.m. UTC | #10
> Am 02.09.2019 um 12:37 schrieb Richard Biener <richard.guenther@gmail.com>:
> 
> On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>> 
>>>> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
>>>> 
>>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>>>> 
>>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>>>>> 
>>>>>> Bootstrap and regtest running on x86_64-redhat-linux and
>>>>>> s390x-redhat-linux.
>>>>>> 
>>>>>> This patch series adds signaling FP comparison support (both scalar and
>>>>>> vector) to s390 backend.
>>>>> 
>>>>> I'm running into a problem on ppc64 with this patch, and it would be
>>>>> great if someone could help me figure out the best way to resolve it.
>>>>> 
>>>>> vector36.C test is failing because gimplifier produces the following
>>>>> 
>>>>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>>>>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>>>>> 
>>>>> from
>>>>> 
>>>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>>>>>                { -1, -1, -1, -1 } ,
>>>>>                { 0, 0, 0, 0 } >
>>>>> 
>>>>> Since the comparison tree code is now hidden behind a temporary, my code
>>>>> does not have anything to pass to the backend.  The reason for creating
>>>>> a temporary is that the comparison can trap, and so the following check
>>>>> in gimplify_expr fails:
>>>>> 
>>>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>>>>>  goto out;
>>>>> 
>>>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
>>>>> operation_could_trap_p (GT).
>>>>> 
>>>>> My current solution is to simply state that backend does not support
>>>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
>>>>> cause performance regressions due to having to fall back to scalar
>>>>> comparisons.
>>>>> 
>>>>> I was thinking about two other possible solutions:
>>>>> 
>>>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>>>>> a bit complicated, because tree_could_throw_p checks not only for
>>>>> floating point traps, but also e.g. for array index out of bounds
>>>>> traps.  So I would have to create a tree_could_throw_p version which
>>>>> disregards specific kinds of traps.
>>>>> 
>>>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>>>>> its tree_code instead of SSA_NAME.  The potential problem I see with
>>>>> this is that there appears to be no guarantee that _5 will be inlined
>>>>> into _6 at a later point.  So if we say that we don't need to fall
>>>>> back to scalar comparisons based on availability of vector >
>>>>> instruction and inlining does not happen, then what's actually will
>>>>> be required is vector selection (vsel on S/390), which might not be
>>>>> available in general case.
>>>>> 
>>>>> What would be a better way to proceed here?
>>>> 
>>>> On GIMPLE there isn't a good reason to split out trapping comparisons
>>>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
>>>> where it is important because we'd have no way to represent EH info
>>>> when not done.  It might be a bit awkward to preserve EH across RTL
>>>> expansion though in case the [VEC_]COND_EXPR are not expanded
>>>> as a single pattern, but I'm not sure.
>>> 
>>> Ok, so I'm testing the following now - for the problematic test that
>>> helped:
>>> 
>>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
>>> index b0c9f9b671a..940aa394769 100644
>>> --- a/gcc/gimple-expr.c
>>> +++ b/gcc/gimple-expr.c
>>> @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
>>>        || TREE_CODE (t) == BIT_FIELD_REF);
>>> }
>>> 
>>> -/*  Return true if T is a GIMPLE condition.  */
>>> +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
>>> 
>>> -bool
>>> -is_gimple_condexpr (tree t)
>>> +static bool
>>> +is_gimple_condexpr_1 (tree t, bool allow_traps)
>>> {
>>>  return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
>>> -                             && !tree_could_throw_p (t)
>>> +                             && (allow_traps || !tree_could_throw_p (t))
>>>                              && is_gimple_val (TREE_OPERAND (t, 0))
>>>                              && is_gimple_val (TREE_OPERAND (t, 1))));
>>> }
>>> 
>>> +/*  Return true if T is a GIMPLE condition.  */
>>> +
>>> +bool
>>> +is_gimple_condexpr (tree t)
>>> +{
>>> +  return is_gimple_condexpr_1 (t, false);
>>> +}
>>> +
>>> +/* Like is_gimple_condexpr, but allow the T to trap.  */
>>> +
>>> +bool
>>> +is_possibly_trapping_gimple_condexpr (tree t)
>>> +{
>>> +  return is_gimple_condexpr_1 (t, true);
>>> +}
>>> +
>>> /* Return true if T is a gimple address.  */
>>> 
>>> bool
>>> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
>>> index 1ad1432bd17..20546ca5b99 100644
>>> --- a/gcc/gimple-expr.h
>>> +++ b/gcc/gimple-expr.h
>>> @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
>>>                                         tree *);
>>> extern bool is_gimple_lvalue (tree);
>>> extern bool is_gimple_condexpr (tree);
>>> +extern bool is_possibly_trapping_gimple_condexpr (tree);
>>> extern bool is_gimple_address (const_tree);
>>> extern bool is_gimple_invariant_address (const_tree);
>>> extern bool is_gimple_ip_invariant_address (const_tree);
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index daa0b71c191..4e6256390c0 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>>  else if (gimple_test_f == is_gimple_val
>>>           || gimple_test_f == is_gimple_call_addr
>>>           || gimple_test_f == is_gimple_condexpr
>>> +        || gimple_test_f == is_possibly_trapping_gimple_condexpr
>>>           || gimple_test_f == is_gimple_mem_rhs
>>>           || gimple_test_f == is_gimple_mem_rhs_or_call
>>>           || gimple_test_f == is_gimple_reg_rhs
>>> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>>          enum gimplify_status r0, r1, r2;
>>> 
>>>          r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>>> -                             post_p, is_gimple_condexpr, fb_rvalue);
>>> +                             post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
>>>          r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
>>>                              post_p, is_gimple_val, fb_rvalue);
>>>          r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>>> index b75fdb2e63f..175b858f56b 100644
>>> --- a/gcc/tree-cfg.c
>>> +++ b/gcc/tree-cfg.c
>>> @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
>>>      return true;
>>>    }
>>> 
>>> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
>>> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
>>> +  if ((rhs_code == VEC_COND_EXPR
>>> +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
>>> +       : (rhs_code == COND_EXPR
>>> +       ? !is_gimple_condexpr (rhs1)
>>> +       : !is_gimple_val (rhs1)))
>>>      || !is_gimple_val (rhs2)
>>>      || !is_gimple_val (rhs3))
>>>    {
>>> 
>>>> 
>>>> To go this route you'd have to split the is_gimple_condexpr check
>>>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
>>>> code (do we have any?) have to be extra careful then.
>>>> 
>>> 
>>> We have expand_vector_condition, which turns VEC_COND_EXPR into
>>> COND_EXPR - but this should be harmless, right?  I could not find
>>> anything else.
>> 
>> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
>> COND_EXPR usages.  There is, of course, a great deal more code, so I'm
>> not sure whether I looked exhaustively through it, but there are at
>> least store_expr and do_jump which do exactly this during expansion.
>> Should we worry about EH edges at this point?
> 
> Well, the EH edge needs to persist (and be rooted off the comparison,
> not the selection).

Ok, I'm trying to create some samples that may reveal problems with EH
edges in these two cases.  So far with these experiments I only managed
to find and unrelated S/390 bug :-)
https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00065.html

> That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs
> (it allows is_gimple_val which isn't proper form for GIMPLE_COND).  Of course
> there's code using it for GIMPLE_CONDs which would need to be adjusted.

I'm sorry, I don't quite get this - what would that buy us?  and what
would you use instead?  Right now we fix up non-conforming values
accepted by is_gimple_val using gimple_cond_get_ops_from_tree - is
there a problem with this approach?

What I have in mind right now is to:
- allow trapping conditions for COND_EXPR and VEC_COND_EXPR;
- report them as trapping in operation_could_trap_p and
  tree_could_trap_p iff their condition is trapping;
- find and adjust all places where this messes up EH edges.

GIMPLE_COND logic appears to be already covered precisely because it
uses is_gimple_condexpr.

Am I missing something?
Richard Biener Sept. 3, 2019, 10:07 a.m. UTC | #11
On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 02.09.2019 um 12:37 schrieb Richard Biener <richard.guenther@gmail.com>:
> >
> > On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>
> >>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >>>
> >>>> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
> >>>>
> >>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>>>
> >>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >>>>>>
> >>>>>> Bootstrap and regtest running on x86_64-redhat-linux and
> >>>>>> s390x-redhat-linux.
> >>>>>>
> >>>>>> This patch series adds signaling FP comparison support (both scalar and
> >>>>>> vector) to s390 backend.
> >>>>>
> >>>>> I'm running into a problem on ppc64 with this patch, and it would be
> >>>>> great if someone could help me figure out the best way to resolve it.
> >>>>>
> >>>>> vector36.C test is failing because gimplifier produces the following
> >>>>>
> >>>>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
> >>>>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
> >>>>>
> >>>>> from
> >>>>>
> >>>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
> >>>>>                { -1, -1, -1, -1 } ,
> >>>>>                { 0, 0, 0, 0 } >
> >>>>>
> >>>>> Since the comparison tree code is now hidden behind a temporary, my code
> >>>>> does not have anything to pass to the backend.  The reason for creating
> >>>>> a temporary is that the comparison can trap, and so the following check
> >>>>> in gimplify_expr fails:
> >>>>>
> >>>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
> >>>>>  goto out;
> >>>>>
> >>>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
> >>>>> operation_could_trap_p (GT).
> >>>>>
> >>>>> My current solution is to simply state that backend does not support
> >>>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
> >>>>> cause performance regressions due to having to fall back to scalar
> >>>>> comparisons.
> >>>>>
> >>>>> I was thinking about two other possible solutions:
> >>>>>
> >>>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
> >>>>> a bit complicated, because tree_could_throw_p checks not only for
> >>>>> floating point traps, but also e.g. for array index out of bounds
> >>>>> traps.  So I would have to create a tree_could_throw_p version which
> >>>>> disregards specific kinds of traps.
> >>>>>
> >>>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
> >>>>> its tree_code instead of SSA_NAME.  The potential problem I see with
> >>>>> this is that there appears to be no guarantee that _5 will be inlined
> >>>>> into _6 at a later point.  So if we say that we don't need to fall
> >>>>> back to scalar comparisons based on availability of vector >
> >>>>> instruction and inlining does not happen, then what's actually will
> >>>>> be required is vector selection (vsel on S/390), which might not be
> >>>>> available in general case.
> >>>>>
> >>>>> What would be a better way to proceed here?
> >>>>
> >>>> On GIMPLE there isn't a good reason to split out trapping comparisons
> >>>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> >>>> where it is important because we'd have no way to represent EH info
> >>>> when not done.  It might be a bit awkward to preserve EH across RTL
> >>>> expansion though in case the [VEC_]COND_EXPR are not expanded
> >>>> as a single pattern, but I'm not sure.
> >>>
> >>> Ok, so I'm testing the following now - for the problematic test that
> >>> helped:
> >>>
> >>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> >>> index b0c9f9b671a..940aa394769 100644
> >>> --- a/gcc/gimple-expr.c
> >>> +++ b/gcc/gimple-expr.c
> >>> @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
> >>>        || TREE_CODE (t) == BIT_FIELD_REF);
> >>> }
> >>>
> >>> -/*  Return true if T is a GIMPLE condition.  */
> >>> +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
> >>>
> >>> -bool
> >>> -is_gimple_condexpr (tree t)
> >>> +static bool
> >>> +is_gimple_condexpr_1 (tree t, bool allow_traps)
> >>> {
> >>>  return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
> >>> -                             && !tree_could_throw_p (t)
> >>> +                             && (allow_traps || !tree_could_throw_p (t))
> >>>                              && is_gimple_val (TREE_OPERAND (t, 0))
> >>>                              && is_gimple_val (TREE_OPERAND (t, 1))));
> >>> }
> >>>
> >>> +/*  Return true if T is a GIMPLE condition.  */
> >>> +
> >>> +bool
> >>> +is_gimple_condexpr (tree t)
> >>> +{
> >>> +  return is_gimple_condexpr_1 (t, false);
> >>> +}
> >>> +
> >>> +/* Like is_gimple_condexpr, but allow the T to trap.  */
> >>> +
> >>> +bool
> >>> +is_possibly_trapping_gimple_condexpr (tree t)
> >>> +{
> >>> +  return is_gimple_condexpr_1 (t, true);
> >>> +}
> >>> +
> >>> /* Return true if T is a gimple address.  */
> >>>
> >>> bool
> >>> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
> >>> index 1ad1432bd17..20546ca5b99 100644
> >>> --- a/gcc/gimple-expr.h
> >>> +++ b/gcc/gimple-expr.h
> >>> @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
> >>>                                         tree *);
> >>> extern bool is_gimple_lvalue (tree);
> >>> extern bool is_gimple_condexpr (tree);
> >>> +extern bool is_possibly_trapping_gimple_condexpr (tree);
> >>> extern bool is_gimple_address (const_tree);
> >>> extern bool is_gimple_invariant_address (const_tree);
> >>> extern bool is_gimple_ip_invariant_address (const_tree);
> >>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> >>> index daa0b71c191..4e6256390c0 100644
> >>> --- a/gcc/gimplify.c
> >>> +++ b/gcc/gimplify.c
> >>> @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >>>  else if (gimple_test_f == is_gimple_val
> >>>           || gimple_test_f == is_gimple_call_addr
> >>>           || gimple_test_f == is_gimple_condexpr
> >>> +        || gimple_test_f == is_possibly_trapping_gimple_condexpr
> >>>           || gimple_test_f == is_gimple_mem_rhs
> >>>           || gimple_test_f == is_gimple_mem_rhs_or_call
> >>>           || gimple_test_f == is_gimple_reg_rhs
> >>> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >>>          enum gimplify_status r0, r1, r2;
> >>>
> >>>          r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
> >>> -                             post_p, is_gimple_condexpr, fb_rvalue);
> >>> +                             post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
> >>>          r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
> >>>                              post_p, is_gimple_val, fb_rvalue);
> >>>          r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
> >>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> >>> index b75fdb2e63f..175b858f56b 100644
> >>> --- a/gcc/tree-cfg.c
> >>> +++ b/gcc/tree-cfg.c
> >>> @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
> >>>      return true;
> >>>    }
> >>>
> >>> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
> >>> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
> >>> +  if ((rhs_code == VEC_COND_EXPR
> >>> +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
> >>> +       : (rhs_code == COND_EXPR
> >>> +       ? !is_gimple_condexpr (rhs1)
> >>> +       : !is_gimple_val (rhs1)))
> >>>      || !is_gimple_val (rhs2)
> >>>      || !is_gimple_val (rhs3))
> >>>    {
> >>>
> >>>>
> >>>> To go this route you'd have to split the is_gimple_condexpr check
> >>>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
> >>>> code (do we have any?) have to be extra careful then.
> >>>>
> >>>
> >>> We have expand_vector_condition, which turns VEC_COND_EXPR into
> >>> COND_EXPR - but this should be harmless, right?  I could not find
> >>> anything else.
> >>
> >> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
> >> COND_EXPR usages.  There is, of course, a great deal more code, so I'm
> >> not sure whether I looked exhaustively through it, but there are at
> >> least store_expr and do_jump which do exactly this during expansion.
> >> Should we worry about EH edges at this point?
> >
> > Well, the EH edge needs to persist (and be rooted off the comparison,
> > not the selection).
>
> Ok, I'm trying to create some samples that may reveal problems with EH
> edges in these two cases.  So far with these experiments I only managed
> to find and unrelated S/390 bug :-)
> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00065.html
>
> > That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs
> > (it allows is_gimple_val which isn't proper form for GIMPLE_COND).  Of course
> > there's code using it for GIMPLE_CONDs which would need to be adjusted.
>
> I'm sorry, I don't quite get this - what would that buy us?  and what
> would you use instead?  Right now we fix up non-conforming values
> accepted by is_gimple_val using gimple_cond_get_ops_from_tree - is
> there a problem with this approach?
>
> What I have in mind right now is to:
> - allow trapping conditions for COND_EXPR and VEC_COND_EXPR;
> - report them as trapping in operation_could_trap_p and
>   tree_could_trap_p iff their condition is trapping;
> - find and adjust all places where this messes up EH edges.
>
> GIMPLE_COND logic appears to be already covered precisely because it
> uses is_gimple_condexpr.
>
> Am I missing something?

Not really - all I'm saying is that currently we use is_gimple_condexpr
to check whether a GENERIC tree is suitable for [VEC_]COND_EXPR
during for example forward propagation.

And GIMPLE_COND already uses its own logic (as you say) but
still passes use is_gimple_condexpr for it.

So my proposal would be to change is_gimple_condexpr to
allow trapping [VEC_]COND_EXPR and stop using is_gimple_condexpr
checks on conditions to be used for GIMPLE_CONDs (and substitute
another predicate there).  For this to work and catch wrong-doings
we should amend gimple_cond_get_ops_from_tree to assert
that the extracted condition cannot trap.

Richard.
Ilya Leoshkevich Sept. 3, 2019, 10:34 a.m. UTC | #12
> Am 03.09.2019 um 12:07 schrieb Richard Biener <richard.guenther@gmail.com>:
> 
> On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>>> Am 02.09.2019 um 12:37 schrieb Richard Biener <richard.guenther@gmail.com>:
>>> 
>>> On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>>> 
>>>>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>>>> 
>>>>>> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
>>>>>> 
>>>>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>>>>>> 
>>>>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
>>>>>>>> 
>>>>>>>> Bootstrap and regtest running on x86_64-redhat-linux and
>>>>>>>> s390x-redhat-linux.
>>>>>>>> 
>>>>>>>> This patch series adds signaling FP comparison support (both scalar and
>>>>>>>> vector) to s390 backend.
>>>>>>> 
>>>>>>> I'm running into a problem on ppc64 with this patch, and it would be
>>>>>>> great if someone could help me figure out the best way to resolve it.
>>>>>>> 
>>>>>>> vector36.C test is failing because gimplifier produces the following
>>>>>>> 
>>>>>>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
>>>>>>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
>>>>>>> 
>>>>>>> from
>>>>>>> 
>>>>>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
>>>>>>>               { -1, -1, -1, -1 } ,
>>>>>>>               { 0, 0, 0, 0 } >
>>>>>>> 
>>>>>>> Since the comparison tree code is now hidden behind a temporary, my code
>>>>>>> does not have anything to pass to the backend.  The reason for creating
>>>>>>> a temporary is that the comparison can trap, and so the following check
>>>>>>> in gimplify_expr fails:
>>>>>>> 
>>>>>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
>>>>>>> goto out;
>>>>>>> 
>>>>>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
>>>>>>> operation_could_trap_p (GT).
>>>>>>> 
>>>>>>> My current solution is to simply state that backend does not support
>>>>>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
>>>>>>> cause performance regressions due to having to fall back to scalar
>>>>>>> comparisons.
>>>>>>> 
>>>>>>> I was thinking about two other possible solutions:
>>>>>>> 
>>>>>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
>>>>>>> a bit complicated, because tree_could_throw_p checks not only for
>>>>>>> floating point traps, but also e.g. for array index out of bounds
>>>>>>> traps.  So I would have to create a tree_could_throw_p version which
>>>>>>> disregards specific kinds of traps.
>>>>>>> 
>>>>>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
>>>>>>> its tree_code instead of SSA_NAME.  The potential problem I see with
>>>>>>> this is that there appears to be no guarantee that _5 will be inlined
>>>>>>> into _6 at a later point.  So if we say that we don't need to fall
>>>>>>> back to scalar comparisons based on availability of vector >
>>>>>>> instruction and inlining does not happen, then what's actually will
>>>>>>> be required is vector selection (vsel on S/390), which might not be
>>>>>>> available in general case.
>>>>>>> 
>>>>>>> What would be a better way to proceed here?
>>>>>> 
>>>>>> On GIMPLE there isn't a good reason to split out trapping comparisons
>>>>>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
>>>>>> where it is important because we'd have no way to represent EH info
>>>>>> when not done.  It might be a bit awkward to preserve EH across RTL
>>>>>> expansion though in case the [VEC_]COND_EXPR are not expanded
>>>>>> as a single pattern, but I'm not sure.
>>>>> 
>>>>> Ok, so I'm testing the following now - for the problematic test that
>>>>> helped:
>>>>> 
>>>>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
>>>>> index b0c9f9b671a..940aa394769 100644
>>>>> --- a/gcc/gimple-expr.c
>>>>> +++ b/gcc/gimple-expr.c
>>>>> @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
>>>>>       || TREE_CODE (t) == BIT_FIELD_REF);
>>>>> }
>>>>> 
>>>>> -/*  Return true if T is a GIMPLE condition.  */
>>>>> +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
>>>>> 
>>>>> -bool
>>>>> -is_gimple_condexpr (tree t)
>>>>> +static bool
>>>>> +is_gimple_condexpr_1 (tree t, bool allow_traps)
>>>>> {
>>>>> return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
>>>>> -                             && !tree_could_throw_p (t)
>>>>> +                             && (allow_traps || !tree_could_throw_p (t))
>>>>>                             && is_gimple_val (TREE_OPERAND (t, 0))
>>>>>                             && is_gimple_val (TREE_OPERAND (t, 1))));
>>>>> }
>>>>> 
>>>>> +/*  Return true if T is a GIMPLE condition.  */
>>>>> +
>>>>> +bool
>>>>> +is_gimple_condexpr (tree t)
>>>>> +{
>>>>> +  return is_gimple_condexpr_1 (t, false);
>>>>> +}
>>>>> +
>>>>> +/* Like is_gimple_condexpr, but allow the T to trap.  */
>>>>> +
>>>>> +bool
>>>>> +is_possibly_trapping_gimple_condexpr (tree t)
>>>>> +{
>>>>> +  return is_gimple_condexpr_1 (t, true);
>>>>> +}
>>>>> +
>>>>> /* Return true if T is a gimple address.  */
>>>>> 
>>>>> bool
>>>>> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
>>>>> index 1ad1432bd17..20546ca5b99 100644
>>>>> --- a/gcc/gimple-expr.h
>>>>> +++ b/gcc/gimple-expr.h
>>>>> @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
>>>>>                                        tree *);
>>>>> extern bool is_gimple_lvalue (tree);
>>>>> extern bool is_gimple_condexpr (tree);
>>>>> +extern bool is_possibly_trapping_gimple_condexpr (tree);
>>>>> extern bool is_gimple_address (const_tree);
>>>>> extern bool is_gimple_invariant_address (const_tree);
>>>>> extern bool is_gimple_ip_invariant_address (const_tree);
>>>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>>>> index daa0b71c191..4e6256390c0 100644
>>>>> --- a/gcc/gimplify.c
>>>>> +++ b/gcc/gimplify.c
>>>>> @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>>>> else if (gimple_test_f == is_gimple_val
>>>>>          || gimple_test_f == is_gimple_call_addr
>>>>>          || gimple_test_f == is_gimple_condexpr
>>>>> +        || gimple_test_f == is_possibly_trapping_gimple_condexpr
>>>>>          || gimple_test_f == is_gimple_mem_rhs
>>>>>          || gimple_test_f == is_gimple_mem_rhs_or_call
>>>>>          || gimple_test_f == is_gimple_reg_rhs
>>>>> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>>>>         enum gimplify_status r0, r1, r2;
>>>>> 
>>>>>         r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>>>>> -                             post_p, is_gimple_condexpr, fb_rvalue);
>>>>> +                             post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
>>>>>         r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
>>>>>                             post_p, is_gimple_val, fb_rvalue);
>>>>>         r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
>>>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>>>>> index b75fdb2e63f..175b858f56b 100644
>>>>> --- a/gcc/tree-cfg.c
>>>>> +++ b/gcc/tree-cfg.c
>>>>> @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
>>>>>     return true;
>>>>>   }
>>>>> 
>>>>> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
>>>>> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
>>>>> +  if ((rhs_code == VEC_COND_EXPR
>>>>> +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
>>>>> +       : (rhs_code == COND_EXPR
>>>>> +       ? !is_gimple_condexpr (rhs1)
>>>>> +       : !is_gimple_val (rhs1)))
>>>>>     || !is_gimple_val (rhs2)
>>>>>     || !is_gimple_val (rhs3))
>>>>>   {
>>>>> 
>>>>>> 
>>>>>> To go this route you'd have to split the is_gimple_condexpr check
>>>>>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
>>>>>> code (do we have any?) have to be extra careful then.
>>>>>> 
>>>>> 
>>>>> We have expand_vector_condition, which turns VEC_COND_EXPR into
>>>>> COND_EXPR - but this should be harmless, right?  I could not find
>>>>> anything else.
>>>> 
>>>> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
>>>> COND_EXPR usages.  There is, of course, a great deal more code, so I'm
>>>> not sure whether I looked exhaustively through it, but there are at
>>>> least store_expr and do_jump which do exactly this during expansion.
>>>> Should we worry about EH edges at this point?
>>> 
>>> Well, the EH edge needs to persist (and be rooted off the comparison,
>>> not the selection).
>> 
>> Ok, I'm trying to create some samples that may reveal problems with EH
>> edges in these two cases.  So far with these experiments I only managed
>> to find and unrelated S/390 bug :-)
>> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00065.html
>> 
>>> That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs
>>> (it allows is_gimple_val which isn't proper form for GIMPLE_COND).  Of course
>>> there's code using it for GIMPLE_CONDs which would need to be adjusted.
>> 
>> I'm sorry, I don't quite get this - what would that buy us?  and what
>> would you use instead?  Right now we fix up non-conforming values
>> accepted by is_gimple_val using gimple_cond_get_ops_from_tree - is
>> there a problem with this approach?
>> 
>> What I have in mind right now is to:
>> - allow trapping conditions for COND_EXPR and VEC_COND_EXPR;
>> - report them as trapping in operation_could_trap_p and
>>  tree_could_trap_p iff their condition is trapping;
>> - find and adjust all places where this messes up EH edges.
>> 
>> GIMPLE_COND logic appears to be already covered precisely because it
>> uses is_gimple_condexpr.
>> 
>> Am I missing something?
> 
> Not really - all I'm saying is that currently we use is_gimple_condexpr
> to check whether a GENERIC tree is suitable for [VEC_]COND_EXPR
> during for example forward propagation.
> 
> And GIMPLE_COND already uses its own logic (as you say) but
> still passes use is_gimple_condexpr for it.
> 
> So my proposal would be to change is_gimple_condexpr to
> allow trapping [VEC_]COND_EXPR and stop using is_gimple_condexpr
> checks on conditions to be used for GIMPLE_CONDs (and substitute
> another predicate there).  For this to work and catch wrong-doings
> we should amend gimple_cond_get_ops_from_tree to assert
> that the extracted condition cannot trap.

Ah, I think now I understand.  While I wanted to keep is_gimple_condexpr
as is and introduce is_possibly_trapping_gimple_condexpr, you're saying
we rather need to change is_gimple_condexpr and introduce, say,
is_non_trapping_gimple_condexpr.

This makes sense, thanks for the explanation!
Richard Biener Sept. 3, 2019, 11:29 a.m. UTC | #13
On Tue, Sep 3, 2019 at 12:34 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 03.09.2019 um 12:07 schrieb Richard Biener <richard.guenther@gmail.com>:
> >
> > On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>
> >>> Am 02.09.2019 um 12:37 schrieb Richard Biener <richard.guenther@gmail.com>:
> >>>
> >>> On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>>
> >>>>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >>>>>
> >>>>>> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
> >>>>>>
> >>>>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>>>>>
> >>>>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >>>>>>>>
> >>>>>>>> Bootstrap and regtest running on x86_64-redhat-linux and
> >>>>>>>> s390x-redhat-linux.
> >>>>>>>>
> >>>>>>>> This patch series adds signaling FP comparison support (both scalar and
> >>>>>>>> vector) to s390 backend.
> >>>>>>>
> >>>>>>> I'm running into a problem on ppc64 with this patch, and it would be
> >>>>>>> great if someone could help me figure out the best way to resolve it.
> >>>>>>>
> >>>>>>> vector36.C test is failing because gimplifier produces the following
> >>>>>>>
> >>>>>>> _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 };
> >>>>>>> _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
> >>>>>>>
> >>>>>>> from
> >>>>>>>
> >>>>>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) ,
> >>>>>>>               { -1, -1, -1, -1 } ,
> >>>>>>>               { 0, 0, 0, 0 } >
> >>>>>>>
> >>>>>>> Since the comparison tree code is now hidden behind a temporary, my code
> >>>>>>> does not have anything to pass to the backend.  The reason for creating
> >>>>>>> a temporary is that the comparison can trap, and so the following check
> >>>>>>> in gimplify_expr fails:
> >>>>>>>
> >>>>>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p))
> >>>>>>> goto out;
> >>>>>>>
> >>>>>>> gimple_test_f is is_gimple_condexpr, and it eventually calls
> >>>>>>> operation_could_trap_p (GT).
> >>>>>>>
> >>>>>>> My current solution is to simply state that backend does not support
> >>>>>>> SSA_NAME in vector comparisons, however, I don't like it, since it may
> >>>>>>> cause performance regressions due to having to fall back to scalar
> >>>>>>> comparisons.
> >>>>>>>
> >>>>>>> I was thinking about two other possible solutions:
> >>>>>>>
> >>>>>>> 1. Change the gimplifier to allow trapping vector comparisons.  That's
> >>>>>>> a bit complicated, because tree_could_throw_p checks not only for
> >>>>>>> floating point traps, but also e.g. for array index out of bounds
> >>>>>>> traps.  So I would have to create a tree_could_throw_p version which
> >>>>>>> disregards specific kinds of traps.
> >>>>>>>
> >>>>>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use
> >>>>>>> its tree_code instead of SSA_NAME.  The potential problem I see with
> >>>>>>> this is that there appears to be no guarantee that _5 will be inlined
> >>>>>>> into _6 at a later point.  So if we say that we don't need to fall
> >>>>>>> back to scalar comparisons based on availability of vector >
> >>>>>>> instruction and inlining does not happen, then what's actually will
> >>>>>>> be required is vector selection (vsel on S/390), which might not be
> >>>>>>> available in general case.
> >>>>>>>
> >>>>>>> What would be a better way to proceed here?
> >>>>>>
> >>>>>> On GIMPLE there isn't a good reason to split out trapping comparisons
> >>>>>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs
> >>>>>> where it is important because we'd have no way to represent EH info
> >>>>>> when not done.  It might be a bit awkward to preserve EH across RTL
> >>>>>> expansion though in case the [VEC_]COND_EXPR are not expanded
> >>>>>> as a single pattern, but I'm not sure.
> >>>>>
> >>>>> Ok, so I'm testing the following now - for the problematic test that
> >>>>> helped:
> >>>>>
> >>>>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> >>>>> index b0c9f9b671a..940aa394769 100644
> >>>>> --- a/gcc/gimple-expr.c
> >>>>> +++ b/gcc/gimple-expr.c
> >>>>> @@ -602,17 +602,33 @@ is_gimple_lvalue (tree t)
> >>>>>       || TREE_CODE (t) == BIT_FIELD_REF);
> >>>>> }
> >>>>>
> >>>>> -/*  Return true if T is a GIMPLE condition.  */
> >>>>> +/* Helper for is_gimple_condexpr and is_possibly_trapping_gimple_condexpr.  */
> >>>>>
> >>>>> -bool
> >>>>> -is_gimple_condexpr (tree t)
> >>>>> +static bool
> >>>>> +is_gimple_condexpr_1 (tree t, bool allow_traps)
> >>>>> {
> >>>>> return (is_gimple_val (t) || (COMPARISON_CLASS_P (t)
> >>>>> -                             && !tree_could_throw_p (t)
> >>>>> +                             && (allow_traps || !tree_could_throw_p (t))
> >>>>>                             && is_gimple_val (TREE_OPERAND (t, 0))
> >>>>>                             && is_gimple_val (TREE_OPERAND (t, 1))));
> >>>>> }
> >>>>>
> >>>>> +/*  Return true if T is a GIMPLE condition.  */
> >>>>> +
> >>>>> +bool
> >>>>> +is_gimple_condexpr (tree t)
> >>>>> +{
> >>>>> +  return is_gimple_condexpr_1 (t, false);
> >>>>> +}
> >>>>> +
> >>>>> +/* Like is_gimple_condexpr, but allow the T to trap.  */
> >>>>> +
> >>>>> +bool
> >>>>> +is_possibly_trapping_gimple_condexpr (tree t)
> >>>>> +{
> >>>>> +  return is_gimple_condexpr_1 (t, true);
> >>>>> +}
> >>>>> +
> >>>>> /* Return true if T is a gimple address.  */
> >>>>>
> >>>>> bool
> >>>>> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h
> >>>>> index 1ad1432bd17..20546ca5b99 100644
> >>>>> --- a/gcc/gimple-expr.h
> >>>>> +++ b/gcc/gimple-expr.h
> >>>>> @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *,
> >>>>>                                        tree *);
> >>>>> extern bool is_gimple_lvalue (tree);
> >>>>> extern bool is_gimple_condexpr (tree);
> >>>>> +extern bool is_possibly_trapping_gimple_condexpr (tree);
> >>>>> extern bool is_gimple_address (const_tree);
> >>>>> extern bool is_gimple_invariant_address (const_tree);
> >>>>> extern bool is_gimple_ip_invariant_address (const_tree);
> >>>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> >>>>> index daa0b71c191..4e6256390c0 100644
> >>>>> --- a/gcc/gimplify.c
> >>>>> +++ b/gcc/gimplify.c
> >>>>> @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >>>>> else if (gimple_test_f == is_gimple_val
> >>>>>          || gimple_test_f == is_gimple_call_addr
> >>>>>          || gimple_test_f == is_gimple_condexpr
> >>>>> +        || gimple_test_f == is_possibly_trapping_gimple_condexpr
> >>>>>          || gimple_test_f == is_gimple_mem_rhs
> >>>>>          || gimple_test_f == is_gimple_mem_rhs_or_call
> >>>>>          || gimple_test_f == is_gimple_reg_rhs
> >>>>> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >>>>>         enum gimplify_status r0, r1, r2;
> >>>>>
> >>>>>         r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
> >>>>> -                             post_p, is_gimple_condexpr, fb_rvalue);
> >>>>> +                             post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue);
> >>>>>         r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
> >>>>>                             post_p, is_gimple_val, fb_rvalue);
> >>>>>         r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
> >>>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> >>>>> index b75fdb2e63f..175b858f56b 100644
> >>>>> --- a/gcc/tree-cfg.c
> >>>>> +++ b/gcc/tree-cfg.c
> >>>>> @@ -4121,8 +4121,11 @@ verify_gimple_assign_ternary (gassign *stmt)
> >>>>>     return true;
> >>>>>   }
> >>>>>
> >>>>> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
> >>>>> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
> >>>>> +  if ((rhs_code == VEC_COND_EXPR
> >>>>> +       ? !is_possibly_trapping_gimple_condexpr (rhs1)
> >>>>> +       : (rhs_code == COND_EXPR
> >>>>> +       ? !is_gimple_condexpr (rhs1)
> >>>>> +       : !is_gimple_val (rhs1)))
> >>>>>     || !is_gimple_val (rhs2)
> >>>>>     || !is_gimple_val (rhs3))
> >>>>>   {
> >>>>>
> >>>>>>
> >>>>>> To go this route you'd have to split the is_gimple_condexpr check
> >>>>>> I guess and eventually users turning [VEC_]COND_EXPR into conditional
> >>>>>> code (do we have any?) have to be extra careful then.
> >>>>>>
> >>>>>
> >>>>> We have expand_vector_condition, which turns VEC_COND_EXPR into
> >>>>> COND_EXPR - but this should be harmless, right?  I could not find
> >>>>> anything else.
> >>>>
> >>>> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also
> >>>> COND_EXPR usages.  There is, of course, a great deal more code, so I'm
> >>>> not sure whether I looked exhaustively through it, but there are at
> >>>> least store_expr and do_jump which do exactly this during expansion.
> >>>> Should we worry about EH edges at this point?
> >>>
> >>> Well, the EH edge needs to persist (and be rooted off the comparison,
> >>> not the selection).
> >>
> >> Ok, I'm trying to create some samples that may reveal problems with EH
> >> edges in these two cases.  So far with these experiments I only managed
> >> to find and unrelated S/390 bug :-)
> >> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00065.html
> >>
> >>> That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs
> >>> (it allows is_gimple_val which isn't proper form for GIMPLE_COND).  Of course
> >>> there's code using it for GIMPLE_CONDs which would need to be adjusted.
> >>
> >> I'm sorry, I don't quite get this - what would that buy us?  and what
> >> would you use instead?  Right now we fix up non-conforming values
> >> accepted by is_gimple_val using gimple_cond_get_ops_from_tree - is
> >> there a problem with this approach?
> >>
> >> What I have in mind right now is to:
> >> - allow trapping conditions for COND_EXPR and VEC_COND_EXPR;
> >> - report them as trapping in operation_could_trap_p and
> >>  tree_could_trap_p iff their condition is trapping;
> >> - find and adjust all places where this messes up EH edges.
> >>
> >> GIMPLE_COND logic appears to be already covered precisely because it
> >> uses is_gimple_condexpr.
> >>
> >> Am I missing something?
> >
> > Not really - all I'm saying is that currently we use is_gimple_condexpr
> > to check whether a GENERIC tree is suitable for [VEC_]COND_EXPR
> > during for example forward propagation.
> >
> > And GIMPLE_COND already uses its own logic (as you say) but
> > still passes use is_gimple_condexpr for it.
> >
> > So my proposal would be to change is_gimple_condexpr to
> > allow trapping [VEC_]COND_EXPR and stop using is_gimple_condexpr
> > checks on conditions to be used for GIMPLE_CONDs (and substitute
> > another predicate there).  For this to work and catch wrong-doings
> > we should amend gimple_cond_get_ops_from_tree to assert
> > that the extracted condition cannot trap.
>
> Ah, I think now I understand.  While I wanted to keep is_gimple_condexpr
> as is and introduce is_possibly_trapping_gimple_condexpr, you're saying
> we rather need to change is_gimple_condexpr and introduce, say,
> is_non_trapping_gimple_condexpr.
>
> This makes sense, thanks for the explanation!

I'd say is_gimple_cond_expr - bah! stupid clashing names ;)
OK, so is_gimple_cond_condexpr vs. is_gimple_condexpr_cond?
Hmm, no.

I don't like to explicitely spell "non_trapping" here but instead
find names that tell one is for GIMPLE_COND while the
other is for GIMPLE_ASSIGN with a [VEC_]COND_EXPR operation.

Ah, maybe is_gimple_cond_condition () vs. is_gimple_assign_condition ()?
Or is_gimple_condexpr_for_cond and is_gimple_condexpr_for_assign?

Richard.